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

perf: use browserslist targets #6891

Merged
merged 9 commits into from
Jul 26, 2024
Merged

Conversation

VIKTORVAV99
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 commented Jun 22, 2024

Issue

ESBuild is not picking up these automatically creating less than optimal generated code.

Description

Adds a package that converts our browserslist target to a ESBuild target instead, this shaves some code of the bundles but more importantly uses more efficient syntax and newer features like optional chaining to reduce property access and similar.

This reduces the total app size from 4307.86 KiB to 4294.74 KiB.

Also converts the config file to a module config instead of a cts config so the new package works as expected. This is needed for Vite 5 later on anyhow.

Note

If we are unsure about the compatibility I can always adjust the browsers list targets as well. For example explicitly include iOS/Safari 15.

Double check

  • I have run pnpx prettier@2 --write . and poetry run format in the top level directory to format my changes.

Comment on lines +132 to +134
...(mode === 'test'
? []
: [
Copy link
Member Author

Choose a reason for hiding this comment

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

This is either ESLint or Prettiers doing but it should be fine.

@madsnedergaard
Copy link
Member

I'm a bit on edge about this, seems like a very low gain compared to potentially reducing usability in older browsers 🤔

@VIKTORVAV99
Copy link
Member Author

I'm a bit on edge about this, seems like a very low gain compared to potentially reducing usability in older browsers 🤔

We are already targeting the CSS of the browsers list config via autoprefixer and postcss. And the current target we have is quite broad so I'm not that worried about it.

@tonypls
Copy link
Collaborator

tonypls commented Jul 8, 2024

I'm a bit on edge about this, seems like a very low gain compared to potentially reducing usability in older browsers 🤔

We are already targeting the CSS of the browsers list config via autoprefixer and postcss. And the current target we have is quite broad so I'm not that worried about it.

Could we test this branch preview vs prod on old browsers somehow? browser stack or similar?

@VIKTORVAV99
Copy link
Member Author

I'm a bit on edge about this, seems like a very low gain compared to potentially reducing usability in older browsers 🤔

We are already targeting the CSS of the browsers list config via autoprefixer and postcss. And the current target we have is quite broad so I'm not that worried about it.

Could we test this branch preview vs prod on old browsers somehow? browser stack or similar?

I can look into it 👍🏻

@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Jul 18, 2024

Gave it a quick test on Safari 15.6 using browserstack (latest supported version of safari 15) and it seems to work without issues. But to be sure this doesn't change silently in the future we can add safari 15 to the browsers list target.

I see little point in testing chromium and Firefox as these are self updating, we could test on Firefox ESR if you wish but I'm not sure we need to support that. People that run ESR (which is a choice in this case) are not many and probably not our target audience.

EDIT: Firefox ESR are included in the defaults.

@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Jul 22, 2024

I propose we use this browsers list config:

  "browserslist": [
    "defaults",
    "safari 15.6",
    "ios_saf 15.6",
    "> 0.25%",
    "not ie 11",
    "not op_mini all"
  ],

vs what we use now:

  "browserslist": [
    "defaults",
    "not ie 11",
    "not op_mini all"
  ],

Copy link
Collaborator

@tonypls tonypls left a comment

Choose a reason for hiding this comment

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

Looks good to me!

To be safe I think we could document a revert plan then monitor sentry on deployment, if we see a significantly higher than expected error rate we could roll it back.

@VIKTORVAV99
Copy link
Member Author

Looks good to me!

To be safe I think we could document a revert plan then monitor sentry on deployment, if we see a significantly higher than expected error rate we could roll it back.

Should I change the browsers list target to explicitly include Safari 15.6?

@VIKTORVAV99 VIKTORVAV99 enabled auto-merge (squash) July 26, 2024 09:19
@VIKTORVAV99 VIKTORVAV99 merged commit 52c1362 into master Jul 26, 2024
21 checks passed
@VIKTORVAV99 VIKTORVAV99 deleted the vik/use_browserslist_targets branch July 26, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file frontend 🎨 performance 🏎
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants