Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

Remove use of setImmediate from fbjs #2127

Open
thomassuckow opened this issue Jul 11, 2019 · 7 comments
Open

Remove use of setImmediate from fbjs #2127

thomassuckow opened this issue Jul 11, 2019 · 7 comments

Comments

@thomassuckow
Copy link

thomassuckow commented Jul 11, 2019

Draft-js uses non-complaint setImmediate polyfill from fbjs. fbjs has had open tickets to fix this issue for years, it's time to assume it will never be fixed and stop using it.

fbjs assumes there is a global called global and attempts to polyfill setimmediate on it. Then fbjs tries to expose this polyfilled function incorrectly. In spec conformant environments there is no global called global and a runtime error occurs from trying to access a nonexistent variable.

Note the tc39 proposal where they got global from changed to globalThis nearly a year ago.

Why not just setTimeout(fn, 0)? It is only being used as a hack anyway for other event handlers to fire first.

@thomassuckow thomassuckow reopened this Jul 11, 2019
@thomassuckow thomassuckow changed the title fbjs Remove use of setImmediate from fbjs Jul 11, 2019
@claudiopro
Copy link
Contributor

Maybe @zpao can shed some light on the direction of fbjs and future plans for these polyfills?

@mrkev
Copy link
Contributor

mrkev commented Dec 18, 2019

@suavelizard
Copy link

Just to add another non-legacy use-case this is happening in electron when using AntD.

@ptolomeus
Copy link

Why not just setTimeout(fn, 0)? It is only being used as a hack anyway for other event handlers to fire first.

Because it may be not fast enough?
If you compare setTimeout(callback, 0) with setImmediate(callback) you should see, that setTimeout waits more then 0ms before the callback gets executed.

You can see the difference here.

var timeouts = 250;
var timeout = function(){
    --timeouts ? window.setTimeout(timeout, 0) : console.timeEnd('setTimeout');
}
console.time('setTimeout');
timeout();

var immediates = 250;
var immediate = function(){
    --immediates ? window.setImmediate(immediate) : console.timeEnd('setImmediate');
}
console.time('setImmediate');
immediate();

If you like to try out the snippet above, I suggest you to do it in the browser console of the demo page, where the setImmediate is pollyfilled for browsers which don't support it natively

The point, where I 100% agree is, that it is really sad, that the fbjs doesn't fix their setImmediate export...

@thomassuckow
Copy link
Author

or just import https://www.npmjs.com/package/setimmediate themselves.

@ptolomeus
Copy link

ptolomeus commented May 20, 2020

I've created a pull request facebook/fbjs#387, which should fix most of the issues related to the setImmediate/global things. So let's see if it helps... At least it makes draft-js usable in IE11 🙏

@tianzhich
Copy link

2 years later since facebook/fbjs#290, any quick solution here?

facebook-github-bot pushed a commit that referenced this issue Oct 19, 2020
Summary:
This diff fixes the iPad double-space bug.

It involved a few false starts and I went down the a couple rabbit holes but I finally arrived at the fact this setImmediate isn't needed at all. In fact, the whole of pending state seems to not be necessary.

Except for IE.

It makes sense really. `editor.update` should only be called once per change (semantically, what it does is call `props.onChange`). `editOnBeforeInput` and `editOnInput` should be treated as two parts of the same function, just maybe letting the DOM do something in-between.

What the callback in this setImmedate does is really a subset of what the onInput event does, and it's useless everywhere EXCEPT for IE, because IE doesn't fire input events (and React doesn't polyfill this).

Since this is very IE specific, in a future diff I'd like to explore using something else, since our `setImmedate` polyfill is not only apparently [non compliant](#2127), it also causes some [typechecking issues](#1943) extenrally. IE 11 supports mutation observers, so we could use that instead. This should also reduce bundle size since we wouln't be pulling down that polyfill.

But anyway, that's a diff to go up the stack. For now lets just get this out.

Reviewed By: steveluscher, asiandrummer

Differential Revision: D24368500

fbshipit-source-id: 589ebef35b6beac881995db8a9de0bfccc940334
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants