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

Feat / Extend minimum browser support #519

Merged

Conversation

langemike
Copy link
Collaborator

@langemike langemike commented Apr 25, 2024

We want to support a broader user base by extending our browser support to chrome68 and up.

Vite internally uses ['es2020', 'edge88', 'firefox78', 'chrome87', 'safari14']. So the only difference is the supported chrome browsers. See https://vitejs.dev/guide/build#browser-compatibility for reference.

I only needed to write a CSS fallback for the dvh unit because it was the only unsupported CSS feature we have ran into.
This change also contains a variable injection fix for index.html which has been introduced (with the upgrade to vite 5, I assume).

We also did some internal experiments with @vitejs/plugin-legacy, but this change has a bigger impact on our bundle/build. If we even want to broaden our browser support further than chrome68 then @vitejs/plugin-legacy would definitely be a viable option.

I've tested this on Chrome 68 using browserstack.

Our original PR: Videodock#186
Our @vitejs/plugin-legacy experiment PR (for reference): Videodock#182

@AntonLantukh
Copy link
Collaborator

@langemike is "Chrome 68" something you need for Smart TV devices?

@langemike
Copy link
Collaborator Author

@langemike is "Chrome 68" something you need for Smart TV devices?

Correct!

@@ -1,3 +1,5 @@
import 'core-js/es/array/flat-map';
import 'core-js/es/object/from-entries';
Copy link
Collaborator

Choose a reason for hiding this comment

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

@langemike do you think we can import it conditionally based on the environment variable (APP_LEGACY_BROWSER)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would not limit this env-var only to the conditional imports, which makes the approach more close like I originally intended, but I think we should keep it simple for now (at least) and make it more flexible if use-cases of demand for this increases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My main concern here is the "polyfills" file we additionally get when we add Chrome legacy support. Even if we don't need it.

Copy link
Collaborator Author

@langemike langemike May 29, 2024

Choose a reason for hiding this comment

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

I revised the approach. Legacy support is now opt-in by providing APP_LEGACY_BUILD=1 during yarn build.

A polyfills.js is still created because some dependencies have imported core-js internally.

Without legacy support (smaller files):

build/public/manifest.json                           0.49 kB │ gzip:   0.29 kB
build/public/index.html                              2.97 kB │ gzip:   0.81 kB
build/public/assets/default_avatar-D3fd0lZl.png     26.96 kB
build/public/assets/style-xyq8hHtg.css             164.70 kB │ gzip:  29.50 kB
build/public/assets/polyfills-BTIFuIxg.js          108.89 kB │ gzip:  43.26 kB │ map:   676.28 kB
build/public/assets/react-Dl1qj1q7.js              143.24 kB │ gzip:  46.11 kB │ map:   351.28 kB
build/public/assets/inplayer-Caq7sMti.js           313.25 kB │ gzip:  92.39 kB │ map:   708.10 kB
build/public/assets/index-BJFB1YLe.js              342.80 kB │ gzip: 103.81 kB │ map: 1,260.17 kB
build/public/assets/vendor-oOs1bjTA.js           1,615.64 kB │ gzip: 431.98 kB │ map: 4,926.55 kB

With APP_LEGACY_BUILD=1 (bigger files):

build/public/manifest.json                           0.49 kB │ gzip:   0.29 kB
build/public/index.html                              2.97 kB │ gzip:   0.81 kB
build/public/assets/default_avatar-D3fd0lZl.png     26.96 kB
build/public/assets/style-xyq8hHtg.css             164.70 kB │ gzip:  29.50 kB
build/public/assets/polyfills-DtCB2TJL.js          90.71 kB │ gzip:  37.81 kB │ map:   567.16 kB
build/public/assets/react-Ba7Lyyxn.js              143.28 kB │ gzip:  46.12 kB │ map:   351.34 kB
build/public/assets/inplayer-CHHsr38C.js           313.25 kB │ gzip:  92.39 kB │ map:   708.10 kB
build/public/assets/index-CuUwpo7s.js              343.22 kB │ gzip: 104.00 kB │ map: 1,260.34 kB
build/public/assets/vendor-DQoT3ipD.js           1,615.76 kB │ gzip: 432.01 kB │ map: 4,926.75 kB

Copy link
Collaborator

Choose a reason for hiding this comment

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

For some reason they are smaller with APP_LEGACY_BUILD, but the approach itself looks good 👍 Thank you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the file listings are switched in the comment above. The added polyfills make the polyfills file larger when APP_LEGACY_BUILD is enabled.

@langemike langemike force-pushed the feat/vite-minimum-browser-support branch 2 times, most recently from dc1022c to bc6b79a Compare May 29, 2024 15:53
@ChristiaanScheermeijer
Copy link
Collaborator

We discussed the knip ignore and virtual + relative path import via Slack and are planning to make the following (hopefully last) changes:

  • Update import to import 'virtual:polyfills';
  • Restore knip config and add virtual:polyfills to the ignoreDependencies
  • Update the Vite plugin with:
    resolveId(id) {
      if (id.includes('virtual:polyfills')) {
        return '\0' + id;
      }
    },
    load(id) {
      if (id.includes('\0virtual:polyfills')) {
        return enabled ? `import './polyfills';` : 'export default {};';
      }
    },

@langemike langemike force-pushed the feat/vite-minimum-browser-support branch from bc6b79a to 0e7bb82 Compare June 3, 2024 12:14
@langemike
Copy link
Collaborator Author

@ChristiaanScheermeijer I ammeded my previous commit and performed the approach you suggested.

I removed the ignore knip-config and put virtual:polyfills as a peerDependency to fix Knip's Unlisted dependencies warning.

Copy link
Collaborator

@ChristiaanScheermeijer ChristiaanScheermeijer left a comment

Choose a reason for hiding this comment

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

🙌

@langemike langemike merged commit 1794113 into jwplayer:develop Jun 4, 2024
9 checks passed
@royschut royschut deleted the feat/vite-minimum-browser-support branch June 5, 2024 08:00
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