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.

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

deps: upgrade to esbuild #327

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

deps: upgrade to esbuild #327

wants to merge 7 commits into from

Conversation

shakyShane
Copy link
Collaborator

@shakyShane shakyShane commented May 30, 2023

Reviewer: @GioSensation
Asana:

Description

  • ✅ removes all grunt-related packages
  • ✅ uses esbuild instead of browserify to bundle now
  • ✅ is much, much faster to build - results vary, but the biggest wins will be slower machines and CI
  • ✅ added a watch-mode (npm start) in case you want it. Personally I just let the build script before playwright run because this is so fast, but there might be situations when working in native, where you'll want to build on every file change
  • ❌ I didn't get chance to drop babel and the hundreds of packages that come for the ride, that's because of jest and the way it does it's own transformations on everything 😭 . We can solve that another time if needed (and probably get a speed boost in the meantime), but it's not a blocker for this PR

Steps to test

  • if the test suit passes, we should be pretty confident, but of course it needs testing in some real scenarios
    • UPDATE: tested on real devices, all good
  • note: I used esbuild settings that we discovered recently to work in Catalina, so I'm confident we'll have a good baseline of support.

@shakyShane
Copy link
Collaborator Author

shakyShane commented May 30, 2023

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @shakyShane and the rest of your teammates on Graphite Graphite

@shakyShane shakyShane changed the title maintenance: npm dependencies deps: upgrade to esbuild Sep 26, 2023
@shakyShane shakyShane marked this pull request as ready for review September 26, 2023 15:13
@shakyShane shakyShane force-pushed the shane/npm-deps branch 2 times, most recently from 3adce5b to f2fd5e4 Compare March 7, 2024 11:36
@shakyShane shakyShane changed the base branch from main to 03-07-remove_device-specifc_isEnabled_checks March 7, 2024 11:36
@shakyShane shakyShane force-pushed the 03-07-remove_device-specifc_isEnabled_checks branch from 3d7f66a to 7207a7b Compare March 7, 2024 13:35
@shakyShane shakyShane changed the base branch from 03-07-remove_device-specifc_isEnabled_checks to main March 7, 2024 15:15
scripts/copy-assets.js Outdated Show resolved Hide resolved
Comment on lines 50 to 54

// don't allow tests to run until the background page is ready
if (context.backgroundPages().length === 0) {
await new Promise((resolve) => context.on('backgroundpage', resolve))
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this solves the flakiness with the extension tests. Sometimes the background page is already spawn, sometimes not. This ensures we never proceed with it

Shane Osbourne added 2 commits May 7, 2024 16:05
@sjbarag
Copy link
Contributor

sjbarag commented May 7, 2024

Rebased on 9caa4e4 (Ema/more minor fixes (#556), 2024-05-03) — seems to still work locally. What's the plan for getting this merged? I'd love to use esbuild for autofill and be able to import a .json file directly (without having to mess around with grunt)

@shakyShane
Copy link
Collaborator Author

👋🏻 @sjbarag - thanks for checking in on this. I made more progress during the last 'quick wins' and the very final 'blocker' was a performance regression on very large DOM counts.

My theory is that because of the way esbuild is more aggressive with hoisting modules during the bundling phase, I think more codepaths are hit than would be otherwise. That, or the memory overhead from the eager evaluation of the hoisted modules could be causing it too.

For reference, it was something like 50ms over a page with 1000 inputs, so actually wasn't a worry so to speak. But since that was discovered at the very end of the last quick wins, we decided not to press the button.

I now think that was a mistake, and that any kind of perf hit here would never materialize in reality (we have lots of checks in place to prevent it).

IMO, if the rebased version of this still works on all platforms (eg: spinning them up for a sanity check) then I'd be confident to merge this and move forward.

@GioSensation would you tend to agree - feels like a plaster we need to rip off :)

@sjbarag
Copy link
Contributor

sjbarag commented May 8, 2024

@shakyShane thanks for the context! TBH I didn't do any manual testing (just the existing unit/integration suites), so I can't comment on whether there's any noticeable perf regressions.

@GioSensation
Copy link
Member

GioSensation commented May 9, 2024

I wasn't able to reproduce any performance issue whatsoever, even with the unrealistic page. Not quite sure what we were seeing back then. We can resolve the conflicts, sanity check on all platforms, and merge 🎉. @sjbarag will you do the honor?

@GioSensation
Copy link
Member

And just to celebrate, besides the more modern tooling, better DX, and faster build time, the new output files are ~21% smaller than the old ones. 🚀

Thank you Shane for the original PR and Sean for resurrecting the convo.

@shakyShane
Copy link
Collaborator Author

shakyShane commented May 9, 2024

Thanks @GioSensation - this is a nice step forward.

For more context too, the following none-SERP projects all use esbuild now

  • Autoconsent
  • Autofill (when this merges)
  • Privacy Dashboard
  • All of our extensions
  • Specials pages within CSS, like Duck Player, Onboarding etc
  • There's more I believe...

🥳

CC: @muodov @sammacbeth @jonathanKingston

@GioSensation
Copy link
Member

@sjbarag Please hold on merging this. We may need an urgent fix for Android and I'd rather avoid adding risk at this stage. I'll let you know later today or tomorrow.

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

Successfully merging this pull request may close these issues.

3 participants