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

Preact 7 Support for Internet Explorer 11 #453

Closed
pahund opened this issue Dec 15, 2016 · 38 comments
Closed

Preact 7 Support for Internet Explorer 11 #453

pahund opened this issue Dec 15, 2016 · 38 comments

Comments

@pahund
Copy link

pahund commented Dec 15, 2016

With preact 6.4.0, my medium-sized React application worked just fine on Internet Explorer 11. With preact 7, I get a "stack overflow" message on IE's web developer console.

Are there any plans to make preact 7 work with IE11? I can understand that preact only supports modern browsers, but for better or worse, IE11 is a modern browser, and I can't rule out the possibility that my customers use it :-)

@developit
Copy link
Member

Yes! Absolutely. There's no good reason for Preact to drop IE9+ support, it just didn't block the 7.1 release. If you're able to screenshot the stack trace or any other context, that might be useful. Related issue: #430

@pahund
Copy link
Author

pahund commented Dec 15, 2016

Sure, here is the screenshot:

windows_10

It's a German IE11, the message translates to "out of stack space".

IE version 11.576.14393.0, update version 11.0.38; Windows 10.

My app uses these packages:

"preact": "^7.1.0",
"preact-compat": "^3.9.4",
"react-modal": "^1.5.2",
"react-redux": "^4.4.6",
"recompose": "^0.20.2",
"redux": "^3.6.0",
"redux-saga": "^0.13.0",
"whatwg-fetch": "^2.0.1",
"babel-polyfill": "^6.2.0"

When I get rid of all the redux stuff and just use Preact to render a simple <h1>Hello World</h1>, I get the same error.

When I use preact 6.4.0 instead of 7.1.0, the problem goes away.

@developit
Copy link
Member

Alright, will look into it. It's due to a change in TextNode behavior, which will either be reverted or modified to use non-empty Textnodes.

@developit
Copy link
Member

@pahund Having trouble reproducing this. Updating to preact 7.1 and running preact-boilerplate seems to work fine in IE (checked in 9 - maybe it's specific to 11?)
screen shot 2016-12-15 at 6 21 28 pm

@mkxml
Copy link
Collaborator

mkxml commented Dec 15, 2016

Checked in 11, seems OK too. Ran tests with karma in IE11 too. The only thing I found was #430.

@pahund
Copy link
Author

pahund commented Dec 16, 2016

I have set up a demo project that should let you reproduce the problem: https://github.com/pahund/preact-ie11-bug

I suspect it has something to do with the babel-polyfill, which I need for my project so that promises and generators work on IE11

@developit
Copy link
Member

Ahhhh yes it will be the Promise polyfill. Interesting. To me that seems like an issue with babel-polyfill.

FWIW, I'd recommend using regenerator and promise-polyfill instead of babel-polyfill. Promise-polyfill is a more accurate implementation of Microtasks, and won't trigger the infinite recursion issue.

@developit
Copy link
Member

A quick test to see if we're on the right track would be:

import { options } from 'preact';
options.debounceRendering = f => f();

This will turn off async and bypass the promise usage entirely.

@NekR
Copy link
Collaborator

NekR commented Dec 17, 2016

@developit do you think preact will be working with this Promises polyfill https://github.com/jridgewell/PJs?

@NekR
Copy link
Collaborator

NekR commented Dec 17, 2016

This will turn off async and bypass the promise usage entirely.

Probably makes sense only for browser which don't support promises, which is IE only. IE is slow anyway.

@developit
Copy link
Member

It should work well, since it uses postMessage to defer execution. It will not perform quote as well as some other implementations but it should be very reasonable in IE.

@developit
Copy link
Member

@NekR another option would be to use setTimeout for IE, a decent compromise:

import { options } from 'preact';
options.debounceRendering = setTimeout;

@NekR
Copy link
Collaborator

NekR commented Dec 17, 2016

another option would be to use setTimeout for IE, a decent compromise:

Maybe makes since to have it default so? Or it's already if promises aren't available? If so, then fix is simply load/require/include promises polyfill after preact.

@developit
Copy link
Member

The thing is, babel-polyfill is installing a global Promise that Preact can't distinguish from the native one. Normally, Promise.then() is far and away the best debounce mechanism we have, since it's a cheap way to get access to DOM Microtasks.

Here's a benchmark: https://esbench.com/bench/55d4d44e2efbca1100bf7251
(note that MessageChannel appears fastest, but that is due to batching. I should upate the bench)

@NekR
Copy link
Collaborator

NekR commented Dec 18, 2016

@developit what I mean is that obviously not everyone uses babel-polyfill or any Promise polyfill at all. What does Preact in this case? Does it fallback to setTimeout automatically?

@developit
Copy link
Member

@NekR yep. If Promise isnt available it falls to setTimeout.

@tomatau
Copy link

tomatau commented Jan 20, 2017

Has there been any progress on this?

Asking consumers of our package to all revert from babel-polyfill and use promise-polyfill is a no go for our situation. We're building a shared component that gets dropped into many applications and Preact is perfect as it can keep the shared component size down. We bundle our component using babel but without any polyfills, targeting node and umd keeping preact as an 'external'. So it's up to the consumer how they polyfill.

I've tested as a consumer using babel-polyfill and setting options.debounceRendering = setTimeout but it doesn't solve the problem

Out of stack space

@developit
Copy link
Member

developit commented Jan 20, 2017

@tomatau I think the best course of action here is to cut a 7.2 release containing the already-merged IE fixes. If that doesn't address the recursion issue itself, then we'll need to determine if something more drastic is needed to deal with babel-polyfill's implementation of Mictotasks.

The fact that setTimeout is still letting the recursion happen means it's probably more related to the issue fixed on master. Let's hope that's the case!

@developit
Copy link
Member

developit commented Jan 23, 2017

This should be fixed in 7.2.0 (beta). Let me know if the recursion issue is still happening!

@pahund
Copy link
Author

pahund commented Jan 30, 2017

I can confirm that the bug is fixed in version 7.2.0 (beta). I added preact 7.2.0 to my the demo project I had setup to show the issue. It works fine now, no more “stack overflow” error in Internet Explorer 11's JavaScript console.

Thank you, keep up the good work!

@tomatau
Copy link

tomatau commented Jan 30, 2017

@developit I'm planning to use the 7.2 release in the coming weeks (just convincing our product owner to let me spike the fix which should be fine). Can you hold off closing this until checking my use case which wasn't solved by the options.debounceRendering = setTimeout approach.

@developit
Copy link
Member

@tomatau I closed it 7 days ago, but I totally see your point. Want to open up a new issue specifically for the recursion issue?

@tomatau
Copy link

tomatau commented Jan 31, 2017

Oh no worries. I'll just open a new issue if it definitely is still an issue :)

joeldenning added a commit to CanopyTax/canopy-styleguide that referenced this issue Jan 31, 2017
joeldenning added a commit to CanopyTax/canopy-styleguide that referenced this issue Jan 31, 2017
* Adding preactToCustomElement helper function

* Self review

* more self review

* Classlist polyfill

* More self review

* Attribute values that are set before connected

* Simplification

* Preact@7.1 didn't work in IE11 so upgrading to 7.2. See preactjs/preact#453

* Fixing IE11

* Small fix to cps-button

* Forgot to commit a file
joeldenning added a commit to CanopyTax/canopy-styleguide that referenced this issue Feb 3, 2017
* Adding preactToCustomElement helper function

* Self review

* more self review

* Classlist polyfill

* More self review

* Attribute values that are set before connected

* Simplification

* Preact@7.1 didn't work in IE11 so upgrading to 7.2. See preactjs/preact#453

* Fixing IE11

* Small fix to cps-button

* Forgot to commit a file

* cps-tooltip custom element

* Tests and events.

* Docs

* Polyfilling CustomEvent in IE11 for tests

* Fixing when tooltip is too far to the right

* Oops forgot to uncomment
@hartshorne
Copy link

I found this issue because this error started firing IE11 users after I tried switching from react to preact. I can't reproduce it locally, but am seeing it in production error logs. Switching to preact 7.2.0 did not seem to fix the problem. I get three variants of it, stacktraces from rollbar below:

From setSymbolDesc(this, tag, createDesc(1, value));:

Error: Out of stack space

File "webpack:///./~/core-js/modules/es6.symbol.js" line 139 col 1 in t
setSymbolDesc(this, tag, createDesc(1, value));

File "webpack:///./~/core-js/modules/es6.symbol.js" line 139 col 1 in t
setSymbolDesc(this, tag, createDesc(1, value));

File "webpack:///./~/core-js/modules/es6.symbol.js" line 139 col 1 in t
setSymbolDesc(this, tag, createDesc(1, value));

....

From if(this === ObjectProto)$set.call(OPSymbols, value);:

Error: Out of stack space

File "webpack:///./~/core-js/modules/es6.symbol.js" line 137 col 1 in t
if(this === ObjectProto)$set.call(OPSymbols, value);

File "webpack:///./~/core-js/modules/es6.symbol.js" line 139 col 1 in t
setSymbolDesc(this, tag, createDesc(1, value));

File "webpack:///./~/core-js/modules/es6.symbol.js" line 139 col 1 in t
setSymbolDesc(this, tag, createDesc(1, value));

...

From return hasOwnProperty.call(it, key);:

Error: Out of stack space

File "webpack:///./~/core-js/modules/_has.js" line 3 col 1 in e.exports
return hasOwnProperty.call(it, key);

File "webpack:///./~/core-js/modules/es6.symbol.js" line 138 col 1 in t
if(has(this, HIDDEN) && has(this[HIDDEN], tag))this[HIDDEN][tag] = false;

File "webpack:///./~/core-js/modules/es6.symbol.js" line 139 col 1 in t
setSymbolDesc(this, tag, createDesc(1, value));

File "webpack:///./~/core-js/modules/es6.symbol.js" line 139 col 1 in t
setSymbolDesc(this, tag, createDesc(1, value));

Apologies if this is a core-js issue, it's hard to parse https://github.com/zloirock/core-js#caveats-when-using-symbol-polyfill and zloirock/core-js#260 (comment).

Let me know what I can do to narrow this down.

@developit
Copy link
Member

Hmm - I would strongly recommend not using the Symbol polyfill. It will cause Preact to use Symbol in IE, which it was not designed to do (it checks for Symbol support and uses a fallback if its not supported).

@developit
Copy link
Member

developit commented Feb 8, 2017

@hartshorne It might be worth looking into dropping Symbol from Preact. It adds weight and I'm not sure it's really necessary. Maybe that's the ticket we should open, particularly if it might fix the issue you're running into.

@steadicat
Copy link

@developit yeah it’s probably a good idea to remove the use of Symbol, if possible. The Symbol polyfill is required by Babel when transpiling anything that uses iterables, like for...of or [...iterable]. But Preact uses Symbol.for, which is apparently impossible to polyfill correctly.

Should @hartshorne or I open a new issue for this?

@developit
Copy link
Member

Yup, plus it's already falling back to a pretty reasonable default, which is just an obscure string. One thing we'll need to be careful with - there are a few other libraries (mainly preact-compat) relying on that well-known Symbol. Those will need to be changed too.

I'd be happy to review a PR if you have the time. Right now the Symbol is a constant. I'm okay with that, but really there's no reason we can't just inline it. That might actually speed up property access in V8 👍 . It's only used in 4 places.

@hartshorne
Copy link

We've worked around this by setting our TypeScript target to ES5 for now. Because this change will affect other libraries, I think it's best to leave it to you to manage it. Thank you!

@developit
Copy link
Member

Works for me! I think we'll pull Symbol in Preact 8, I'll add it to the list of breaking changes.

@tomatau
Copy link

tomatau commented Feb 10, 2017

I'm still getting the error for a Webpack UMD bundled preact component that is then bundled into a larger web app. We can get around the problem by using the "var" bundle instead of "umd" for the component but it would be nice for the umd component to work in IE11 without the "out of stack space" issue.

@developit
Copy link
Member

oh - that's interesting.... maybe you are getting 2 copies of Preact?

@tomatau
Copy link

tomatau commented Feb 17, 2017

The UMD bundle has Preact configured as an external so there shouldn't be two copies

@developit
Copy link
Member

developit commented Feb 17, 2017

Ah ok. In that case where/how is preact loaded?
(sorry to dig, just trying to wrap my head around things)

@StoneCypher
Copy link

oh god, this is such a bad idea, but 😅

it can be done like this

@shawnwall
Copy link

In case anyone else gets here.. I'm seeing this issue with preact x beta and either es6-promise or core-js for polyfilling promise.

@marvinhagemeister
Copy link
Member

@shawnwall Can create a new issue with the error you're getting? We're running our test suite against IE11 with each PR that is merged. We're using the same es6-promsie polyfill.

@shawnwall
Copy link

@marvinhagemeister sorry for the false alarm. I believe the error is actually related to the code in a component I am using using Portals, etc. I've created a simplified iframe component myself and now things are okay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants