fix: Restrict officially supported browserlist #2159
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2159 +/- ##
==========================================
- Coverage 46.68% 46.67% -0.02%
==========================================
Files 691 692 +1
Lines 38588 38620 +32
Branches 9622 9809 +187
==========================================
+ Hits 18016 18025 +9
+ Misses 20561 20542 -19
- Partials 11 53 +42
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -1,4 +1,6 @@ | |||
| # Use the default value for browsers we support https://browsersl.ist/#q=defaults | |||
| # Used by post-css only, JS browser targeting is controlled by Vite https://vitejs.dev/config/build-options.html#build-target | |||
There was a problem hiding this comment.
Take a look at this comment you removed (about how this is just for post-css...). We need to wire up Vite to use .browserslistrc in place of build.target using the browserslist-to-esbuild module it looks like: vitejs/vite#11489
Otherwise this .browserslistrc is only going to be used for CSS, and not used when building JS.
A blog post about Vite and browserslist may be information as well: https://dev.to/meduzen/when-vite-ignores-your-browserslist-configuration-3hoe
We should also be able to compare the package size before/after and see the impact (check the size of the code-studio/build folder after doing an npm run build before and after).
|
@mofojed Size Comparison of Before changes: 56.4 MB |
There was a problem hiding this comment.
Update the web-client-ui README with the browsers we support/link to this file: https://github.com/deephaven/web-client-ui?tab=readme-ov-file#browser-support
@dsmmcken was there any other testing you wanted to be done for this?
|
No, I had no specific testing requirements. Which browser doesn't support top level await? |
|
@dsmmcken This link outlines which versions of which browsers can use top level await https://caniuse.com/mdn-javascript_operators_await_top_level , it seems that with our new restrictions, the browsers and versions that we have outlined should support top level await |
Closes #1752
** The only thing I am not 100% sure about is how we can test this before merging **
I tried experimenting with checking the compatible browsers on their own website: https://browsersl.ist/