[docs] Add polyfill for IE 11 #13324
Conversation
|
@eps1lon |
This is clearly not the case. I don't really care if we use polyfills or not in the demos but issues can not surface if we don't run tests on the docs. That we use ESNext features does not contradict our pledge to support IE11 in our core components. How users transform/polyfill their code is up to them. Sure it's probably better to write What is way worse is that we apparently support IE11 in the core but our docs might break in those browsers. Again this was not a proposal for a specific implementation but remark that our docs have an issue. The PR was just for demonstration. |
What I mean is that we can reproduce the issue with the documentation demos. If the button requires a polyfill that we provide in the docs, people won't see it in our docs, they will only notice it in their app. They can't link the docs as a reproduction, they need to provide a codesandbox. For context, we didn't have codesandbox for a long time. It's easier now, so it's less important, I agree. Also, I have been considering for a long time to add Sentry on our documentation.
Yes, we really need to fix that! @mui-org/core-contributors How do you guys want to handle the problem? |
oliviertassinari
force-pushed
the
docs/add-polyfill
branch
from
e86e72b to
522dd66
Compare
| @@ -43,6 +43,7 @@ const styles = theme => ({ | |||
| }, | |||
| fabButton: { | |||
| position: 'absolute', | |||
| zIndex: 1, | |||
pages/_document.js
Outdated
| @@ -67,8 +69,7 @@ ga('create', '${config.google.id}', 'material-ui.com'); | |||
| `, | |||
| }} | |||
| /> | |||
| <NextScript /> | |||
| <script async src="https://cdn.jsdelivr.net/docsearch.js/2/docsearch.min.js" /> | |||
| <script defer src="https://cdn.polyfill.io/v2/polyfill.js?features=Array.prototype.findIndex" /> | |||
There was a problem hiding this comment.
oliviertassinari
force-pushed
the
docs/add-polyfill
branch
2 times, most recently
from
7a4570a to
b828ea1
Compare
oliviertassinari
changed the title
oliviertassinari
force-pushed
the
docs/add-polyfill
branch
from
b828ea1 to
36a1d24
Compare
oliviertassinari
force-pushed
the
docs/add-polyfill
branch
from
36a1d24 to
ce81eb8
Compare
|
After, looks good |
pages/_document.js
Outdated
| {/* Dynamic polyfill for IE 11 */} | ||
| <script | ||
| defer | ||
| src="https://cdn.polyfill.io/v2/polyfill.min.js?features=Array.prototype.findIndex" |
There was a problem hiding this comment.
Wouldn't it be better to import the polyfill in the actual file so that next.js will only use it in that chunk? RIght now this is still somewhat dangerous when people land on pages that require that polyfill.
There was a problem hiding this comment.
The polyfill is only needed for dynamic interactions. It's a tradeoff, this way, we can keep using findIndex without bloating everybody bundles.
| @@ -75,7 +75,7 @@ class HorizontalLinearStepper extends React.Component { | |||
| } | |||
|
|
|||
| this.setState(state => { | |||
| const skipped = new Set(state.skipped.values()); | |||
| const skipped = new Set(state.skipped); | |||
There was a problem hiding this comment.
that is still new Set(iterable) which is not supported in IE11: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set#Browser_compatibility
I guess the underlying problem is that IE 11 does not implement the iterator protocol via @@iterator.
There was a problem hiding this comment.
It's the demo working on IE 11?
There was a problem hiding this comment.
It is. Either IE 11 fast tracks this somehow and not considers this an iterable or the MDN data is outdated.
There was a problem hiding this comment.
Just tested it in the console and it doesn't.
The hole skipped logic is currently dead code since it is only responsible for https://github.com/mui-org/material-ui/blob/ce81eb82cb616c1585c98bf882877ab5aaeb4c4b/docs/src/pages/demos/steppers/HorizontalLinearStepper.js#L111-L113
but steps are not completed by default https://github.com/mui-org/material-ui/blob/ce81eb82cb616c1585c98bf882877ab5aaeb4c4b/packages/material-ui/src/Step/Step.js#L153
There was a problem hiding this comment.
Ok, it doesn't work 😢
var s1 = new Set()
s1.add('a')
console.log(s1.has('a'))
var s2 = new Set(s2)
console.log(s2.has('a'))outputs
true
false
@eps1lon Well spotted 👍
pages/_document.js
Outdated
| {/* Dynamic polyfill for IE 11 */} | ||
| <script | ||
| defer | ||
| src="https://cdn.polyfill.io/v2/polyfill.min.js?features=Array.prototype.findIndex" |
There was a problem hiding this comment.
Wouldn't it be better to import the polyfill in the actual file so that next.js will only use it in that chunk? RIght now this is still somewhat dangerous when people land on pages that require that polyfill.
oliviertassinari
force-pushed
the
docs/add-polyfill
branch
from
ce81eb8 to
a7cec5b
Compare
oliviertassinari
force-pushed
the
docs/add-polyfill
branch
from
a7cec5b to
b9cd7b6
Compare
TL;DR:
Fixes the following issues in ie11:
Opened PR instead of issue so that reviewers have access to deploy preview and see if
yarn sizecatches bundle size increase.Current behavior
Our docs are deployed as JS that can be parsed by every browser listed in our
.browserlistsrcExpected
Our docs are deployed as JS that is equivalent in all browsers in our
.browserlistrcContext
Thanks to
@babel/preset-envand.browserlistrcthe syntax in our demos is transpiled so that it can be understood by mainstream browsers. However we don't have any polyfills (other thanfetch). That provide functionality that was added with es6 and es7.Since we don't have any compat tests for our docs things like
Object.valuesornew Set(iterable)can easily slip by. We can't expect from contributors to know about all of those and expect from reviewers to catch those.Solutions
eslint-plugin-compat
At first glance
eslint-plugin-compatsounds like an easy solution but will never catch every compat issue because it does not have access to type information so bultin prototype polyfills can not be identified perfectly. However it is possible to catch those overzealous which is annoying for a linter.useBultins with entry
@babel/preset-envwithuseBuiltins: 'entry'will polyfill every feature that was added in es6 and beyond and is not (correctly) implemented by the browsers in.browserlistrc. However this adds around 400KB uncompressed.This option will add the following polyfills:
useBultins with usage
useBuiltins: 'usage'is currently flagged as experimental but is probably the best tradeoff. It's still overzealous (which it will always be due to missing type information). For example it polyfillsstring.prototype.linkevery time alinkproperty is accessed. I think this will not catch usage in other packages.This option will add the following polyfills:
Just fix the code™
This is what web development was >10 years ago. Current offenders are
Object.values,new Set(iterable),Array.prototype.findIndex.Object.keysis most likeley safe. I think there is some difference in the implementation concerning Symbols in some browsers but that doesn't affect our usage I believe.Array.prototype.includesis only used in server side render so we should be fine there.