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

[docs] Investigate IE 11 crash #14946

Closed
wants to merge 2 commits into from
Closed

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Mar 18, 2019

Investigating #14774

git bisect good ba4332d00a5f951cfc33dcffa5924cc7999b8f05
# first bad commit: [440c9f6320d669319b5a3524b2705d73e795e698] [core] Upgrade dependencies and devDependencies (#14493)

440c9f6 regenerated the whole lockfile. Might be just next might be some other dependency. Might be a good idea to branch of ba4332d and increment by canaries instead of majors.

Already spent a couple of hours on this issue. Working with next.js is pretty frustrating so I'll doubt I spend much more time on the issue. Maybe it will fix itself someday.

No idea why netlify is crashing. It's working locally.

@eps1lon eps1lon added bug 🐛 Something doesn't work docs Improvements or additions to the documentation labels Mar 18, 2019
@eps1lon eps1lon force-pushed the fix/docs/ie11-crash-bad branch from 466cd85 to 392f6b3 Compare March 18, 2019 20:41
@eps1lon eps1lon force-pushed the fix/docs/ie11-crash-bad branch from 392f6b3 to f3cb65b Compare March 18, 2019 21:15
// Use the same helper as Babel to avoid bundle bloat.
import 'core-js/modules/es6.array.find-index';
import 'core-js/modules/es6.map';
import 'core-js/modules/es6.set';
Copy link
Member Author

Choose a reason for hiding this comment

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

This should've been there in the first place: https://reactjs.org/docs/javascript-environment-requirements.html

const entry = async () => {
const entries = await originalEntry();

if (entries['main.js'] && !entries['main.js'].includes('./pages/polyfills.js')) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@mui-pr-bot
Copy link

mui-pr-bot commented Mar 18, 2019

Details of bundle changes.

Comparing: 10438a1...9ebd2a2

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core 0.00% 0.00% 357,579 357,579 91,073 91,073
@material-ui/core/Paper 0.00% 0.00% 68,634 68,634 19,979 19,979
@material-ui/core/Paper.esm 0.00% 0.00% 62,366 62,366 19,060 19,060
@material-ui/core/Popper 0.00% 0.00% 30,454 30,454 10,525 10,525
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 17,390 17,390 5,730 5,730
@material-ui/core/useMediaQuery 0.00% 0.00% 2,471 2,471 1,042 1,042
@material-ui/lab 0.00% 0.00% 152,165 152,165 44,548 44,548
@material-ui/styles 0.00% 0.00% 53,839 53,839 15,566 15,566
@material-ui/system 0.00% 0.00% 17,143 17,143 4,522 4,522
Button 0.00% 0.00% 90,924 90,924 26,941 26,941
Modal 0.00% 0.00% 84,711 84,711 25,208 25,208
colorManipulator 0.00% 0.00% 3,234 3,234 1,301 1,301
docs.landing 0.00% 0.00% 51,843 51,843 11,349 11,349
docs.main -2.48% -3.26% 649,999 633,876 201,578 195,014
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 305,844 305,844 83,862 83,862

Generated by 🚫 dangerJS against 9ebd2a2

@@ -1,7 +1,3 @@
// Use the same helper as Babel to avoid bundle bloat.
import 'core-js/modules/es6.array.find-index';
import 'core-js/modules/es6.set';
Copy link
Member Author

Choose a reason for hiding this comment

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

This created a different environment depending on the bundle. You can verify this by adding a Symbol polyfill. React elements will have sometimes types that are numbers, sometimes Symbols depending on the time and place they were created.

@oliviertassinari oliviertassinari self-assigned this Mar 21, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 21, 2019

I have looked deeper into the problem. I believe that we just have to wait for the release of cssinjs/jss#1065.

I have found the following reproduction:
🔴 KO

import 'docs/src/modules/components/bootstrap';
// --- Post bootstrap -----
import React from 'react';
import { makeStyles } from '@material-ui/styles';

const useStyles = makeStyles({
  root: {
    flex: '1 0 100%',
  },
});

const B = () => {
  useStyles();
  return <div>wtf</div>;
};

export default () => <B />;

✳ OK

import 'docs/src/modules/components/bootstrap';
// --- Post bootstrap -----
import React from 'react';
import { makeStyles } from '@material-ui/styles';

const useStyles = makeStyles({
  root: {
//    flex: '1 0 100%',
  },
});

const B = () => {
  useStyles();
  return <div>wtf</div>;
};

export default () => <B />;

@eps1lon
Copy link
Member Author

eps1lon commented Mar 21, 2019

Have you tried building this branch locally or any other idea why netlify is failing?

I guess we should at least add the Map/Set polyfills. I just recently encountered an issue with the codesandbox-client. IE11 essentially has a non-spec compliant implementation where someMap.set().set() is failing because they return undefined from Map.prototype.set.

I remember that we also had some issues where we had to remove new Set([iterable]).

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 21, 2019

@eps1lon I haven't tried this pull request. My workflow was: remove all the pages from /pages at the exception of index.js (to get a fast test iteration cycle, ~1min). Do a dichotomy to find the source of the problem. The minimal changes required to make the page work was: 1. mount a simple div, 2. remove the style rules body.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 21, 2019

Using the IE 11 debugger is tricky. I had to pause on all the exception. There are quite a few exceptions thrown by feature detection logic that should be ignored. The first meaninfuly error reported is the JSS startWith issue.

@eps1lon
Copy link
Member Author

eps1lon commented Mar 21, 2019

Using the IE 11 debugger is tricky. I had to pause on all the exception. There are quite a few exceptions thrown by feature detection logic that should be ignored. The first meaninfuly error reported is the JSS startWith issue.

I feel you. I wish we could disable next.js error pages so that we could actually catch the exception and not just a logged error from componentDidCatch.

But your debug approach was a bit different. While jss might be a issue it was not the original issue. The major version upgrade of next.js was the first one to introduce a change. I never saw an error in IE11 related to startsWith on that commit.

Does the full site work with all flex usages removed?

@oliviertassinari oliviertassinari removed their assignment Mar 23, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 24, 2019

Do you want to keep pushing in this pull request's direction? Upgrading JSS fixes the IE 11 crash of the documentation: #15034.

@eps1lon
Copy link
Member Author

eps1lon commented Mar 24, 2019

We can close. I'll keep an eye on the polyfill issue that's not working in netlify. The broken Map/Set polyfill might cause trouble in the future.

@eps1lon eps1lon closed this Mar 24, 2019
@oliviertassinari
Copy link
Member

Yes, it has already caused trouble in the past, at least for me in Apollo core with IE 11.

@eps1lon eps1lon deleted the fix/docs/ie11-crash-bad branch March 24, 2019 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants