Remove usage of the ... operator in BackHandler

[dantman]

This usage of ... currently causes BackHandler to silently become non-functional when a Symbol polyfill is used.

Why?

• The [...obj] operator implicitly calls the object's iterator to fill the array
• react-native's internal Set polyfill has a load order issue that breaks it whenever a project uses a Symbol polyfill
• This means that when BackHandler tries to [...set] a Set of subscriptions the result is always an empty array instead of an array of subscriptions

Additionally it's worth noting that the current code is also wastefully inefficient as in order to reverse iterate over subscriptions it:

• Clones the Set (which fills it by implicitly running the set's iterator)
• Uses [...set] to convert the cloned set into an array (which also implicitly runs the iterator on the clone)
• Finally reverses the order of the array

---

This code fixes this problem by replacing the use of multiple Set instance iterators with a simple .forEach loop that unshifts directly into the final array.

Test Plan

I've tested this by opening the repo's RNTester app on Android and tvOS ensuring that the back handler works before changes, the application crashes when I introduce an error (to verify my code changes are being applied to the app), the back handler works and after changes.

Fixes #11968

[pull-bot]

	Warnings
⚠️	📋 Release Notes - This PR appears to be missing Release Notes.

@facebook-github-bot label Needs more information

Generated by 🚫 dangerJS

[henrikra]

You should use map instead of forEach

[dantman]

I used forEach and unshift to skip the extra step of reversing the array.

[facebook-github-bot]

@dantman I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

[dantman]

Yet again the tests fail for some reason that has absolutely nothing to do with my commit.

[janicduplessis]

@dantman What about using Set.prototype.values + Array.prototype.reverse instead? I feed like the intent is clearer there and I'm not sure it will really be slower since I think unshift is relatively slow since it requires moving every item in the array.

[dantman]

😩 If we're going to micro-optimize this I might as well do it right with a benchmark instead of making guesses.

You're right, unshift is actually slow. It's actually faster than the original code with the Set implementation RN is using because of how wasteful the original code is. Though it becomes the slowest when Chrome's native Set implementation is used instead.

However, aside from the unshift, forEach is consistently significantly faster than using an iterator like values(). At least 2x in RN, and 6x when a native Set implementation becomes available.

set.values is only a small improvement over unshift, and it requires the use of Array.from which ends up undoing some of the "clearer intent". Do you want me to use that? Or use the consistently fastest "fixed-array negative forEach method"?

Reverse loop creation comparison using RN Set implementation

https://jsperf.com/reverse-loop-creation-comparison

Reverse loop creation comparison using native Set implementation

Here's a version using native a Set implementation, should it ever become available in the JS engines RN uses.

https://jsperf.com/reverse-loop-creation-comparison-native

[janicduplessis]

@dantman Nice benchmarks :). Since it's not a very hot path I still suggest using values.reverse since that is more readable and avoids using the spread operator + it's still a few times faster than what we had.

[janicduplessis]

Also it looks like you can just do Array.from(theSet), might be a bit faster.

[dantman]

Also it looks like you can just do Array.from(theSet), might be a bit faster.

That I believe would require the set[Symbol.iterator], which is what is broken.

[dantman]

Ready.

[janicduplessis]

Thanks! Thinking about it more we should probably try to fix the thing that makes Set fail when including a Symbol polyfill. Let’s still ship this since it’s really simple and still improves the code.

[janicduplessis]

@facebook-github-bot shipit

[janicduplessis]

@facebook-github-bot shipit

[facebook-github-bot]

@janicduplessis is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[ghost]

changing the BackHandler.android.js doesn't works for me, can anyone please help me in making the back handler work.?

[cosmith]

It's too bad that optimization was prioritized here instead of the removal of set[Symbol.iterator] which was the issue in the first place... I'd say 99% of apps have less than 3 values in _backPressSubscriptions so I don't think we will notice any real-world difference.

Thankfully it's pretty easy to pull the fix in a new BackHandler module, see my answer here: #11968 (comment)

[dantman]

Optimization was not prioritized, the 3rd slowest iteration method was used. Code readability was what was prioritized. And during testing set.values() seemed to work fine (I'd really appreciate some attempt to see what the output of what you're seeing for that is).

My fastest example actually also removes iterators completely.

[janicduplessis]

Would it be possible to fix the set polyfill load order issue instead? I feel like even if we fix it here other similar issues could arise elsewhere.

[dantman]

It's not actually a load order issue. The issue is there is framework host/space (React) and user space (your app's code). User stuff only runs after host stuff. The host (React) polyfills Set and Map but not Symbol. If the user wants Symbol, they can only polyfill it in user space which means the polyfill runs after the host is loaded. However the Set and Map polyfills break if the Symbol (or rather absence of Symbol) is different after they are loaded from before they are loaded.

The fix would be for React Native to ship with a Symbol polyfill. However that is a harder solution, because that's not a matter of just putting a bugfix in RN. That is a matter of changing the decision of what polyfills should be shipped with React Native for all apps, which is something we have to get Facebook to decide.

[janicduplessis]

It should be possible to add the polyfill using metro’s config file, it will run before any other code. See https://github.com/facebook/react-native/blob/master/rn-cli.config.js#L24 and https://github.com/facebook/react-native/blob/master/rn-get-polyfills.js. You can create your own rn-cli.config.js at the root of your project and extend what polyfills are included.

[cosmith]

@dantman sorry that was a bit harsh, I spent half the day upgrading RN to 0.52 to get this commit and it didn't solve my issue :)

_backPressSubscriptions.values() is always null for me when using the core-js/es6/symbol and core-js/fn/symbol/iterator polyfills. Doing _backPressSubscriptions.forEach(sub => subscriptions.unshift(sub)); works. What I didn't understand is that you seemed to have seen this in #15182 (comment) ?

@janicduplessis I'm going to try this approach and report back!

[cosmith]

I gave it a try but couldn't get the bundler to work, it seems require is not yet available so using core-js/es6/symbol fails on require calls. Maybe we would need to use a single file polyfill to get this approach to work...

[janicduplessis]

@cosmith Thanks for trying it out, it sucks that it does not work :(

It seems like RN doesn't load a Set / Map polyfill, are you sure require'ing the polyfill before any other code doesn't work?

import statements get hoisted at the top of the file so to be sure I would do something like in index.js:

require('core-js/es6/symbol');
const React = require('react');
const ReactNative = require('react-native');
...

[dantman]

@janicduplessis RN has its own custom Set & Map polyfills it loads.
https://github.com/facebook/react-native/blob/1e8f3b11027fe0a7514b4fc97d0798d3c64bc895/Libraries/vendor/core/Set.js

These are loaded in the host environment, before index.js is ever required.

[janicduplessis]

@dantman Oh I see, it gets set in InitializeCore, which does effectively run before the main module. Looks like it is configured here

react-native/local-cli/util/Config.js

Line 73 in 654fed4

require.resolve('../../Libraries/Core/InitializeCore'),

, this should also be overridable in rn-cli.config.js maybe adding the polyfills before InitializeCore would work.

[cosmith]

Wouldn't it be easier to just add a Symbol polyfill to RN? I tried the rn-cli.config.js approach but I couldn't get it to load the corejs polyfills.

[RishavKumar-3796]

Guyz please do understand it might not only be the problem with react native. Be careful while integrating it with firebase.
The recent firebase version has the problem of integrating back button in react-native apps!!
Please downgrade the firebase version to firebase-version @5.0.3 and then recheck whether it works or not!
I had the same issue and was worried for days. I finally downgraded to @5.0.3 version and now the back button works perfectly fine!
You may downgrade to lower versions if still facing the problem.