Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Migrate client code to package:web #3610

Merged
merged 7 commits into from
Mar 25, 2024
Merged

Migrate client code to package:web #3610

merged 7 commits into from
Mar 25, 2024

Conversation

parlough
Copy link
Member

No description provided.

parlough
parlough commented Dec 20, 2023
View reviewed changes
web/search.dart Outdated Show resolved Hide resolved
@parlough
Copy link
Member Author

parlough commented Dec 20, 2023 •
edited
Loading

@kevmoo 🦅

I'm not sure if I'm holding the APIs 100% right but most functionality seems to be working fine.

Interestingly, 213KB->205KB despite 1200 extra lines. The interceptor/leaf setup from the old APIs really contributed a lot more than I expected.

Edit: I later removed the use of package:http, sticking with package:web primitives, which reduced this size further :)

@kevmoo
Copy link
Member

kevmoo commented Dec 20, 2023

@parlough – this is due to dropping a BUNCH of dart:html crazy

@parlough parlough mentioned this pull request Dec 29, 2023
Closed
@parlough
Copy link
Member Author

parlough commented Jan 1, 2024 •
edited
Loading

I figured out why the size didn't drop as much as I expected. These three simple uses of package:http increase the final bundle size by ~70KB. I'll rewrite it to just use the web primitives since our uses are simple, but I plan to see if there's any way to reduce how much we bring in within package:http. Most of it comes from package:http_parser and its dependencies: dart-lang/http#1097

@kevmoo
Copy link
Member

kevmoo commented Feb 23, 2024

you still cranking here @parlough ?

@parlough
Copy link
Member Author

parlough commented Feb 23, 2024 •
edited
Loading

Yeah I'm not sure what I was waiting for...but I'll get this updated over the weekend!

@parlough parlough marked this pull request as ready for review February 26, 2024 16:39
@kevmoo kevmoo requested a review from srawlins March 11, 2024 15:48
@srawlins
Copy link
Member

Is this ready @parlough ? Do you have an update on bundle size and use of package:http?

@parlough
Copy link
Member Author

parlough commented Mar 11, 2024 •
edited
Loading

Yep, this is ready for review! Sorry about the delay :)

  • I removed usage of package:http for now, since our few uses of fetch are simple.
  • The largest change to pay attention to is changing the sidebar setup from using a NodeTreeSanitizer for adjusting the links to a more manual method implemented in _updateLinks.
  • There's some misc cleanup, such as consolidating the sidebar-related code to one file and using pattern matching where it makes sense.

After these changes the bundle shrinks from 205KB -> 130KB 🥳

srawlins
srawlins reviewed Mar 11, 2024
View reviewed changes
Copy link
Member

@srawlins srawlins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, @parlough ! The translation mostly looks mechanical, not too crazy.

Can you confirm that the following works in manual testing (we still don't have web tests; #3027):

  1. search, typing, pressing enter to get to the search page, / to trigger search, arrow keys?
  2. "using base href"; I think there are just two cases to test, using and not using. I think that serving the flutter docs is one state, and serving a package's docs use the other state. (I hate that we still have two modes here, but lots to do.)
  3. mobile view; this is the side nav stuff; both the search in the mobile view, and the sidebar loading in the mobile view. I use Chrome's DevTools to emulate mobile.

Thanks again!

@kevmoo
Copy link
Member

kevmoo commented Mar 21, 2024

@parlough ?

@parlough
Copy link
Member Author

Sorry, I've been traveling the past week :)

Yes, I've tested these three scenarios and a few others. They all worked well :)

@kevmoo kevmoo merged commit 79c1675 into dart-lang:main Mar 25, 2024
9 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Mar 26, 2024
@devoncarew
[deps] rev crypto, csslib, dartdoc, http, markdown, web
571ec4e 
Revisions updated by `dart tools/rev_sdk_deps.dart`.

crypto (https://github.com/dart-lang/crypto/compare/f059196..69d13c9):
  69d13c9  2024-03-21  Kevin Moore  Switch sha512 to use fastpath with wasm (dart-archive/crypto#165)

csslib (https://github.com/dart-lang/csslib/compare/b58e487..4216525):
  4216525  2024-03-21  Devon Carew  prep for publishing 1.0.1 (dart-archive/csslib#197)

dartdoc (https://github.com/dart-lang/dartdoc/compare/7be9e24..79c1675):
  79c16759  2024-03-25  Parker Lougheed  Migrate client code to package:web (dart-lang/dartdoc#3610)
  0b1c7fa4  2024-03-25  dependabot[bot]  Bump actions/cache from 4.0.1 to 4.0.2 (dart-lang/dartdoc#3734)
  9fe35ec5  2024-03-20  Sam Rawlins  mustachio: Separate out the context stack LUB type calculation (dart-lang/dartdoc#3730)

http (https://github.com/dart-lang/http/compare/5dfea72..7949d6f):
  7949d6f  2024-03-25  Brian Quinlan  cupertino_http: upgrade ffigen version (dart-lang/http#1159)
  051482a  2024-03-22  Brian Quinlan  Ready cupertino_http for release with WebSocket support (dart-lang/http#1158)
  988b4d4  2024-03-20  Brian Quinlan  Prepare package:cronet_http 1.2 for release (dart-lang/http#1157)
  69f4eff  2024-03-20  Hossein Yousefi  [cronet_http] Upgrade jni to 0.7.3 (dart-lang/http#1156)
  d8b1a9e  2024-03-19  Brian Quinlan  Release `package:web_socket` 0.1.0 (dart-lang/http#1155)
  cfbc191  2024-03-19  Brian Quinlan  Add a WebSocket implementation to package:cupertino_http (dart-lang/http#1153)

markdown (https://github.com/dart-lang/markdown/compare/9c6b1af..8d07abc):
  8d07abc  2024-03-19  MJ Studio  Link uri encoding, URL-escaping should be left alone inside the destination (dart-lang/markdown#598)

web (https://github.com/dart-lang/web/compare/4af904f..c522718):
  c522718  2024-03-20  Kevin Moore  Update MDN documentation (dart-lang/web#213)
  f80dcab  2024-03-15  Srujan Gaddam  Update pubspec description to be consistent with README (dart-lang/web#210)
  27936c4  2024-03-15  Devon Carew  Generate api docs for getters (dart-lang/web#207)
  2f13cd5  2024-03-12  Devon Carew  fix unresolved dartdoc links (dart-lang/web#200)
  686827a  2024-03-12  Srujan Gaddam  Remove reference to static interop and point to dart.dev page for JS interop (dart-lang/web#206)
  9b7e29d  2024-03-12  Devon Carew  Add a 'sourced from mdn docs' line to the MDN sourced dartdoc (dart-lang/web#198)
  51e594b  2024-03-05  Srujan Gaddam  Fix dictionary constructors to accept supertype members and create an empty object when there are no fields (dart-lang/web#197)

Change-Id: Ic90c6f5a7e7d701746276031a8028cdfe76bc27a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/359880
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srawlins srawlins srawlins left review comments

@srujzs srujzs srujzs left review comments

@kevmoo kevmoo kevmoo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@parlough @kevmoo @srawlins @srujzs