Skip to content
New issue

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

#

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

Already on GitHub? # 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
@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-archive/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>
# for free to join this conversation on GitHub. Already have an account? # 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