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

[3.x] Reduce javascript bundle size #604

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Jade-GG
Copy link
Collaborator

@Jade-GG Jade-GG commented Oct 21, 2024

  • Turbo was being eager loaded which takes up ~98kB. I assume we can just lazy load this, I didn't see any issues when trying this.
  • Using eval screws up minification creating a ~30kB size increase. I've changed the check into a function here to get rid of the one and only use of eval in the code, which is a breaking change. I'm not sure why this wasn't done in the first place.

@indykoning
Copy link
Member

indykoning commented Oct 22, 2024

Turbo was being eager loaded which takes up ~98kB. I assume we can just lazy load this, I didn't see any issues when trying this.

Could you add a before and after time for the pageload?
i'm mainly interested in the first contentful paint, total blocing time, and time for the onload event to be fired.

I'm afraid this might reduce that since we boot Vue during the turbo:load event, so booting Vue might be lazy loaded which would improve bundle size but reduce overall pagespeed scores.
If this is the case we might be able to listen to domContentLoaded/onload AND turbo:load

@Jade-GG
Copy link
Collaborator Author

Jade-GG commented Oct 22, 2024

Turbo was being eager loaded which takes up ~98kB. I assume we can just lazy load this, I didn't see any issues when trying this.

Could you add a before and after time for the pageload? i'm mainly interested in the first contentful paint, total blocing time, and time for the onload event to be fired.

I'm afraid this might reduce that since we boot Vue during the turbo:load event, so booting Vue might be lazy loaded which would improve bundle size but reduce overall pagespeed scores. If this is the case we might be able to listen to domContentLoaded/onload AND turbo:load

I've done a few performance tests on my local machine for you to check. Over an average over 5 tests:

  • Lazy loaded

    • First Contentful Paint: 1.42 s (according to lighthouse)
    • Largest Contentful Paint: 2.14 s (according to lighthouse)
    • Total blocking time: 40 ms (according to lighthouse)
    • Time to turbo:load event: 270 ms
  • Eager loaded

    • First Contentful Paint: 1.50 s (according to lighthouse)
    • Largest Contentful Paint: 2.28 s (according to lighthouse)
    • Total blocking time: 90 ms (according to lighthouse)
    • Time to turbo:load event: 245 ms

(Note that the turbo:load time was measured directly in the browser performance console, as opposed to the other metrics which come from the browser's lighthouse console)

What's interesting here is that the actual contentful paints according to lighthouse were consistently 100-200 ms faster, even though turbo actually consistently loaded in 20-30ms later.

EDIT: I've also tested the FCP directly in the browser performance tab as well and actually found no significant difference between the two. Both averaged around 240-250ms with similar fluctuations.

EDIT2: As for the onload event, I'm not sure why but it actually consistently takes over 100ms longer to dispatch on the eager loaded version. In the lazy loaded version it's always fired around the contentful paints, but in the eager loaded version it's always way later for seemingly no reason.

@indykoning
Copy link
Member

indykoning commented Nov 19, 2024

Note: My testing has been done in combination with #646 as i'm pretty sure this will have no/negative impact if we need to wait for the turbo:load event.

Testing has been done on X6 CPU slowdown and fast 4G
(On my macbook without slowdown evaluate modules only takes 20ms, on a live project however only the turbo code takes 33ms during evaluate modules without slowdowns applied)
However bundle size has indeed reduced by about ~30kb gzipped ~100kb regular
Before:
image

After:
image

Everything you see behind and including the block that says Session has been (re)moved.
Including the style recalculation which we indeed want later.

@royduin
Copy link
Member

royduin commented Nov 21, 2024

Is that check used anywhere? And this is approved @indykoning?

@indykoning
Copy link
Member

@Jade-GG if you could update the PR and pull master in, and let us know where that check is used and needs to be fixed 🙂

Other than that, if the tests succeed and if this is merged after #646 (which it is) i approve 👍

indykoning
indykoning previously approved these changes Dec 3, 2024
@Jade-GG
Copy link
Collaborator Author

Jade-GG commented Dec 3, 2024

Said check is used extensively in rapidez/checkout-theme, once in rapidez/account, once in rapidez/multisafepay and probably some of our internal projects as well. It's also in the docs. See this search query

@indykoning
Copy link
Member

Said check is used extensively in rapidez/checkout-theme, once in rapidez/account, once in rapidez/multisafepay and probably some of our internal projects as well. It's also in the docs. See this search query

Okay so the respective 3.x branches should be updated with the check:
https://github.com/rapidez/checkout-theme (master)
rapidez/multisafepay#16 (https://github.com/rapidez/multisafepay/tree/feature/rapidez-v3)
rapidez/account#49 (https://github.com/rapidez/account/tree/feature/rapidez-v3)

And this should probably be added to the upgrade guide:
https://github.com/rapidez/docs/blob/master/src/3.x/upgrading.md

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.

4 participants