Skip to content

convert lib/resources/script.js to Dart #3016

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.

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

Merged
merged 10 commits into from
Apr 11, 2022

Conversation

devoncarew
Copy link
Member

@devoncarew devoncarew commented Apr 8, 2022

Convert lib/resources/script.js to Dart; this PR:

  • removes the scroll position save / restore code
  • moves the highlight.js initialization to Dart code
  • converts the navbar event handling code to dart
  • converts the search box handling code to dart

cc @srawlins

This is more a less a straight 1:1 conversion from the JS code. Some follow up work would be to make the code follow more of a Dart style; later work may change the behavior (improvements to the search box? programmatically building the left nav?).

@devoncarew
Copy link
Member Author

Hmm, it looks like the web/docs.dart signature code has some issues on windows.

@devoncarew
Copy link
Member Author

OK, the sig on windows is incorrect, even after removing line endings. I don't think there are any path char issues - not sure why the platform difference.

  current files: 4862D84995B6C7E2B74C025C713BBF21
  cached sig   : 05FD8D099278ECC2307AAFD453A1E02E
  failed: The web frontend (web/docs.dart) needs to be recompiled; rebuild it with "grind build-web" or "grind build".

@srawlins
Copy link
Member

srawlins commented Apr 8, 2022

It wouldn't be bad if the signature stuff just wasn't run on Windows in CI. It would sort of limit our abilities to accept PRs from a person using Windows, maybe...

@devoncarew
Copy link
Member Author

It wouldn't be bad if the signature stuff just wasn't run on Windows in CI. It would sort of limit our abilities to accept PRs from a person using Windows, maybe...

I think I fixed it; the file listing was in a different order :) I normalize it by sorting the list of files.

@devoncarew
Copy link
Member Author

@srawlins - I believe this is fully functional for what's been converted over, and I've removed the JS code that's been converted. I.e., the functionality is consistent, just some is now in Dart.

Do you want to review the PR now, or, wait until the search stuff is converted?

@srawlins
Copy link
Member

srawlins commented Apr 8, 2022

I think if it's sound, even if in-progress, that sounds great. I can review.

@devoncarew devoncarew marked this pull request as ready for review April 9, 2022 06:40
@devoncarew devoncarew requested a review from srawlins April 9, 2022 06:40
@devoncarew
Copy link
Member Author

devoncarew commented Apr 9, 2022

I think it's ready for review now (I was able to convert over the search code as well).

@srawlins
Copy link
Member

Do we know that this is runnable at pub.dev? Meaning if I cut a release with this PR, and pub.dev used it, the Dart web app is all good to go and run on pub.dev?

I don't have a specific reason why it wouldn't. Since we're already serving up a bit of JS at pub.dev.

@srawlins
Copy link
Member

Actually, the other question might be the more important one: Are dartdocs still visible locally? I am unfamiliar with the sidenav navigation JS.

@devoncarew
Copy link
Member Author

devoncarew commented Apr 11, 2022

Do we know that this is runnable at pub.dev? Meaning if I cut a release with this PR, and pub.dev used it, the Dart web app is all good to go and run on pub.dev?

I would say we're good to go, as long as the pub.dev server doesn't depend on specific JS resources existing in specific places.

cc'ing in @jonasfj and @isoos to comment on whether these changes are compatible w/ pub docs serving. The TLDR from the POV of serving the docs is that the file doc/api/static-assets/script.js will no longer be generated, and instead we'll generate doc/api/static-assets/docs.dart.js. An additional file - docs.dart.js.map will also exist in that assets directory, but it's not strictly speaking necessary to serve that file (definitely ok to though). The generated html files have been updated to refer to the correct resources.

@devoncarew
Copy link
Member Author

Actually, the other question might be the more important one: Are dartdocs still visible locally? I am unfamiliar with the sidenav navigation JS.

Yup, the docs are visible and working locally. There's no real functionality change with this PR (modulo removing the code to save and restore the scroll positions).

@isoos
Copy link
Contributor

isoos commented Apr 11, 2022

he TLDR from the POV of serving the docs is that the file doc/api/static-assets/script.js will no longer be generated, and instead we'll generate doc/api/static-assets/docs.dart.js. An additional file - docs.dart.js.map will also exist in that assets directory, but it's not strictly speaking necessary to serve that file (definitely ok to thought). The generated html files have been updated to refer to the correct resources.

This is OK from pub.dev's PoV. We may need to update a test that checks the list of output files, but that is not unusual.

@jonasfj
Copy link
Member

jonasfj commented Apr 20, 2022

The size increase from 14kb to 444kb makes me a bit sad. I guess it's not a huge concern.

It might make loading the page slower especially on low powered devices. It might be worth checking the lighthouse score before/after..

@isoos
Copy link
Contributor

isoos commented Apr 20, 2022

The size increase from 14kb to 444kb makes me a bit sad. I guess it's not a huge concern.

It is also worth to take a look what makes the size to blow up. We've removed package:intl and replaced the single method we've used with a custom implementation, and gained almost 100 kb in total size (dart-lang/pub-dev#5526)

@devoncarew
Copy link
Member Author

devoncarew commented Apr 20, 2022

We could compile with some minification, and or look into library or code changes to reduce the JS size.

But overall I'm not too worried about it - it's a fixed size increase. I think there are much bigger issues here, and this change is a step towards addressing them (loading in the left nav programmatically, and, improving the search UI).

See #2995 for some serious output size issues w/ dartdoc, and, #3018 for a potential solution (and if we don't land that, we'd get the same benefits from moving to a dynamically generated left nav).

@devoncarew
Copy link
Member Author

The implementation for the web frontend itself is pretty simple: https://github.com/dart-lang/dartdoc/tree/master/web (but still worth looking to see if there are areas contributing to compiled code size).

@parlough
Copy link
Member

Compiling with most optimizations enabled is pretty low hanging fruit: #3028

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants