preactjs / preact

⚛️ Fast 3kB React alternative with the same modern API. Components & Virtual DOM.
https://preactjs.com
MIT License
36.71k stars 1.95k forks source link

Preact 7 Support for Internet Explorer 11 #453

Closed pahund closed 7 years ago

pahund commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

pahund commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

NekR commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

import { options } from 'preact';
options.debounceRendering = setTimeout;
NekR commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

tomatau commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

pahund commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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

hartshorne commented 7 years ago

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 https://github.com/zloirock/core-js/issues/260#issuecomment-262733003.

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

developit commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

tomatau commented 7 years ago

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 commented 7 years ago

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

tomatau commented 7 years ago

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

developit commented 7 years ago

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

StoneCypher commented 5 years ago

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

it can be done like this

shawnwall commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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.