feat(loader): Detect ES6 support & ship appropriate package #53841
Conversation
github-actions
bot
added
Scope: Frontend
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
| @@ -60,6 +62,22 @@ declare const __LOADER__IS_LAZY__: any; | |||
| }); | |||
| } | |||
|
|
|||
| // We detect ES6 support by checking if module scripts are supported | |||
| let _hasEs6Support: boolean | undefined; | |||
| function hasEs6Support(): boolean { | |||
There was a problem hiding this comment.
m: could we check if Symbol is defined globally as an es5 check?
also maybe https://gist.github.com/bendc/d7f3dbc83d0f65ca0433caf90378cd95 works better.
There was a problem hiding this comment.
Hmm, not sure if new Function("(a = 0) => a"); works well with all CSP security settings etc...? isn't this basically eval, which it may complain about?
but I guess we can do 'Symbol' in window check, that should be fine I guess?
FWIW I think module support should be a pretty good proxy. Symbol is a bit "better" supported.
There was a problem hiding this comment.
injecting an inline script seems more likely to trigger CSP fwiw
There was a problem hiding this comment.
good point, we should def. not do that then 😬 so let's go the symbol check route then, wdyt? should this work:
try {
Symbol("loader");
} catch(error) {
// this is ES5
}There was a problem hiding this comment.
This might fail with polyfills but maybe that is ok because it represents that someone is already polyfilling the necessary es6 features.
I'm comfortable with shipping this.
There was a problem hiding this comment.
Yeah, I think if Symbol is polyfilled we can ~assume that ES6 stuff is polyfilled generally. Overall by now ES6 is so widely supported that I figure only very few will be affected by this overall!
| @@ -60,6 +62,22 @@ declare const __LOADER__IS_LAZY__: any; | |||
| }); | |||
| } | |||
|
|
|||
| // We detect ES6 support by checking if module scripts are supported | |||
| let _hasEs6Support: boolean | undefined; | |||
| function hasEs6Support(): boolean { | |||
There was a problem hiding this comment.
This might fail with polyfills but maybe that is ok because it represents that someone is already polyfilling the necessary es6 features.
I'm comfortable with shipping this.
mydea
force-pushed
the
fn/loader-es5-detect
branch
from
2198c77 to
f198b90
Compare
mydea
force-pushed
the
fn/loader-es5-detect
branch
from
f198b90 to
635ae21
Compare
mydea
force-pushed
the
fn/loader-es5-detect
branch
from
635ae21 to
5890960
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #53841 +/- ##
==========================================
+ Coverage 78.31% 79.42% +1.11%
==========================================
Files 5040 5040
Lines 214865 214874 +9
Branches 36495 36495
==========================================
+ Hits 168263 170673 +2410
+ Misses 41266 38806 -2460
- Partials 5336 5395 +59
|
mydea
force-pushed
the
fn/loader-es5-detect
branch
from
5890960 to
79e4f51
Compare
|
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
|
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
|
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
This adds a check for ES6 support into the loader script, and ensures we load the corresponding bundle based on this.
I tested this in IE11 via BrowserStack, where it worked as far as I can tell (a bit hard to try with the free version, but I managed to get it running at least).
Closes #53840