Feat / Manage minimum browser support #186
Conversation
langemike
temporarily deployed
to
Deployment Previews
GitHub Actions
Inactive
langemike
changed the title
langemike
requested review from
ChristiaanScheermeijer,
MelissaDTH and
royschut
platforms/web/vite.config.ts
Outdated
| @@ -114,6 +114,7 @@ export default ({ mode, command }: ConfigEnv): UserConfigExport => { | |||
| cssCodeSplit: false, | |||
| sourcemap: true, | |||
| minify: true, | |||
| target: ['es2020', 'edge88', 'firefox78', 'chrome68', 'safari14'], | |||
There was a problem hiding this comment.
If we want to differentiate between minimum browser support (e.g. a private repo), I suggest using this package: https://github.com/marcofugaro/browserslist-to-esbuild. Where we can load a different .browserslistrc
In the current approach i kept it simple.
| @@ -8,6 +8,7 @@ | |||
| left: 0; | |||
| z-index: variables.$modals-z-index; | |||
| width: 100vw; | |||
| height: 100vh; // Fallback | |||
There was a problem hiding this comment.
Maybe we should elaborate a little bit on what this serves as a fallback for (and the other // Fallback code comments)
There was a problem hiding this comment.
Christiaan and I talked about it and reasoned to keep it short. If you (and others) don't mind, I will submit this to the OTT repo. Because it is immediately placed before the (original) value, I hope the intention of the comment is clear enough.
I'll change it of course if somebody else does think it is necessary.
| @@ -130,6 +131,9 @@ export default ({ mode, command }: ConfigEnv): UserConfigExport => { | |||
| if (id.includes('/node_modules/@inplayer')) { | |||
| return 'inplayer'; | |||
| } | |||
| if (id.includes('/node_modules/core-js')) { | |||
There was a problem hiding this comment.
Yes, perfect! Can/do we need to ensure that this file is loaded before others?
There was a problem hiding this comment.
I followed this FAQ guide https://rollupjs.org/faqs/#why-do-additional-imports-turn-up-in-my-entry-chunks-when-code-splitting and put the polyfills imports on top, based on my recent change. This should ensure they are loaded first.
I thought about bundling the imports in a src/polyfills.tsx file and importing this on top, but I leave that idea open for you or others to decide.
langemike
temporarily deployed
to
Deployment Previews
GitHub Actions
Inactive
langemike
added
the
pr-submitted
langemike
temporarily deployed
to
Deployment Previews
GitHub Actions
Inactive
langemike
temporarily deployed
to
Deployment Previews
GitHub Actions
Inactive
langemike
temporarily deployed
to
Deployment Previews
GitHub Actions
Inactive
langemike
force-pushed
the
feat/vite-minimum-browser-support
branch
from
9555b4e
to
dc1022c
Compare
langemike
temporarily deployed
to
Deployment Previews
GitHub Actions
Inactive
langemike
force-pushed
the
feat/vite-minimum-browser-support
branch
from
dc1022c
to
bc6b79a
Compare
langemike
temporarily deployed
to
Deployment Previews
GitHub Actions
Inactive
langemike
force-pushed
the
feat/vite-minimum-browser-support
branch
from
bc6b79a
to
0e7bb82
Compare
langemike
temporarily deployed
to
Deployment Previews
GitHub Actions
Inactive
langemike
temporarily deployed
to
Deployment Previews
GitHub Actions
Inactive
We want to specifically support browsers from chrome68 and up.
Vite internally uses
['es2020', 'edge88', 'firefox78', 'chrome87', 'safari14']. So the difference is quite small.See: https://vitejs.dev/guide/build#browser-compatibility
I only needed to write a CSS fallback for the
dvhunit because it was the only unsupported CSS feature we have ran into.I have written a detailed explanation why we do not want to use @vitejs/plugin-legacy in this current stage.
I've tested this on Chrome 68 using browserstack.
Ticket: https://videodock.atlassian.net/browse/OTT-1335