Jest setup: Stop polyfilling Promise.

[chrisbobbe]

Hi! 👋 I hope anyone reading this is doing OK these days. Thanks, in any case, for continuing to put your time into the awesome React Native ecosystem.

Summary

It looks like this line was introduced in 3ff3987, in 2015, and it
has remained in a similar form since then. I haven't found any
explanation for it.

At jestjs/jest#10221 [1], a core Jest maintainer says,

"""
As an aside, one should never replace global.Promise [...]. E.g.
when using async-await you will always get the native Promise
regardless of the value of global.Promise.
"""

jestjs/jest#10221 is one issue this line has raised, for anyone
using the latest features of Jest to test async code in their React
Native projects.

[1] jestjs/jest#10221 (comment)

Fixes: #29303

Changelog

[General] [Fixed] - Fix issue with testing async code with Jest's fake timers.

Test Plan

It looks like there's still an effort to keep up-to-date with new versions of the promise NPM package; see b23efc5. But that commit still leaves it a mystery why we're polyfilling Promise here.

I also see Libraries/Core/polyfillPromise.js, but I haven't run into any problems that I can trace to that file.

It's hard to say what side effects might arise from removing the line without knowing what it's for. (#29303 exists mostly to ask that question.)

[facebook-github-bot]

Hi @chrisbobbe!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 6d58490

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,004,694	0
android	hermes	armeabi-v7a	6,668,629	0
android	hermes	x86	7,424,999	0
android	hermes	x86_64	7,315,871	0
android	jsc	arm64-v8a	9,164,411	0
android	jsc	armeabi-v7a	8,820,493	0
android	jsc	x86	9,012,809	0
android	jsc	x86_64	9,589,876	0

Base commit: 6d58490

[andreialecu]

I really hoped this would fix callstack/react-native-testing-library#379 (comment) but after making this change locally everything got broken:

TypeError: TaskQueue: Error with task : Cannot read property 'colors' of undefined

via this hook (from react-native-paper):

  const {colors} = useTheme();

The error does not appear without this change, so it seems there's a legitimate reason for Promise to be polyfilled.

Similar breakage has also been reported here: callstack/react-native-testing-library#379 (comment)

[chrisbobbe]

Similar breakage has also been reported here:

Hmm, I don't see anything there related to react-native-paper. It does look like there's some concerning breakage, though.

so it seems there's a legitimate reason for Promise to be polyfilled.

I'm not sure how legitimate the reason is; possibly the choice wasn't well-considered, and development continued in a way that wasn't compatible with the native Promise, by accident? And nobody realized.

[andreialecu]

There are a bunch of failing tests which seem to hint at some functionality related to pausing promises. This probably can't be done with native promises, hence why a polyfill was used.

https://app.circleci.com/pipelines/github/facebook/react-native/6090/workflows/e4779cac-5fbe-41d7-9bb2-cc1afc89ebab/jobs/164645

[chrisbobbe]

This probably can't be done with native promises

Yeah, possibly. That would be a good question to answer: can the code be changed so that it works with native Promise, and why or why not?

The choice affects React Native's public API; app developers are encouraged to use React Native's Jest preset, which currently activates the polyfill when the developer tests their own app (as we see with jestjs/jest#10221). The choice to restrict app developers to use a particular, non-standard Promise implementation in their tests (and, I guess, in their app code, if they want to avoid surprises where their tests mishandle it) is something that I'd love to see documentation for, at least.

Also, one reason not to replace the global Promise seems pretty fundamental. As I quoted in my opening comment, from a maintainer of Jest, which is also a Facebook project (from jestjs/jest#10221 (comment)),

"""
As an aside, one should never replace global.Promise [...]. E.g.
when using async-await you will always get the native Promise
regardless of the value of global.Promise.
"""

If the polyfill is sometimes used and sometimes not, depending on what syntactic sugar happens to be used, that seems like a likely cause of some pretty hard-to-track bugs—and an obstacle to writing stable async code in a sane way. If React Native has a workaround for that, I'd appreciate documentation for it too. 🙂

[j3tan]

Just wanted to add some information that I encountered while trying to override the global.Promise polyfill to use native promises. There is existing code in react-native that depends on the non-standard promise implementation of done()

Example:

react-native/Libraries/Interaction/InteractionManager.js

Lines 115 to 124 in 1465c8f

	done: (...args) => {
	// $FlowFixMe[method-unbinding] added when improving typing for this parameters
	if (promise.done) {
	return promise.done(...args);
	} else {
	console.warn(
	'Tried to call done when not supported by current Promise implementation.',
	);
	}
	},

which causes issues if you are trying to use InteractionManager.runAfterInteractions(...PromiseTask...) since native promises will throw the following error from TaskQueue if you return a native Promise from the gen function

TypeError: TaskQueue: Error with task task_name: task.gen(...).then(...).catch(...).done is not a function

Due to a chained done() call here:

react-native/Libraries/Interaction/TaskQueue.js

Lines 162 to 178 in 1465c8f

	task
	.gen()
	.then(() => {
	DEBUG &&
	infoLog('TaskQueue: onThen for gen task ' + task.name, {
	stackIdx,
	queueStackSize: this._queueStack.length,
	});
	stackItem.popable = true;
	this.hasTasksToProcess() && this._onMoreTasks();
	})
	.catch(ex => {
	ex.message = `TaskQueue: Error resolving Promise in task ${task.name}: ${ex.message}`;
	throw ex;
	})
	.done();
	}

[robhogan]

Hey @chrisbobbe - sorry I didn't see this PR before creating #34659 . There were some tests that needed updating to work with native Promises, but ultimately I think you're right that this was unnecessary and needed removing.

[andreialecu]

@robhogan I don't want to cause panic here, but I vaguely remember trying to patch RN in the past to use the native promise, and it broke various things that I don't remember right now. :)

It could've affected other libraries, like react-native-testing-library, but again, I forgot the specifics.

There are some assumptions in certain places that the polyfilled promise is being used. See the comment above: #29754 (comment) (although, I see that that was changed by now)

Fingers crossed that #34659 isn't a big breaking change. :)

[robhogan]

Hi @andreialecu - thanks for the heads up. I patched up our internal tests, but you're right that there could be a knock-on in react-native-testing-library.

This is part of an effort to update Jest, where modern timers in recent versions have better Promise handling, so I hope if there are issues created by this, modern Jest might be a way out.

@thymikee, @mdjastrzebski - just FYI, let me know if you foresee an issue here.

[thymikee]

@robhogan we already use node's Promise in our preset which we recommend to our users: https://github.com/callstack/react-native-testing-library/blob/main/jest-preset/index.js