Broken BackAndroid handling #11968

rafales · 2017-01-18T16:31:56Z

We use GitHub Issues for bugs.

Description

Handling back button with BackAndroid does not work - while adding handlers works fine they are never called. The reason behind this - Array.from(new Set([1, 2, 3])) returns empty array. Sets are used by the BackAndroid module.

Reproduction

add new handler in BackAndroid and observe how it's not executed.

Additional Information

React Native version: 0.36, .40
Platform: Android
Operating System: MacOS
Device: Nexus 5, Android 6.0.1, patch lvl: October 5, 2016

The text was updated successfully, but these errors were encountered:

hramos · 2017-01-18T21:11:02Z

Do you have any code samples that show how the error can be reproduced?

dantman · 2017-04-24T06:48:12Z

I have this same issue in RN 0.43.4.

I can confirm that:

The BackAndroid.android.js I see contains var subscriptions = [...backPressSubscriptions].reverse();
In the compiled version turns this becomes var subscriptions = [].concat(babelHelpers.toConsumableArray(backPressSubscriptions)).reverse();
toConsumableArray uses Array.from(arr) for non-arrays
When I dump a log for the expression Array.from(new Set([1, 2, 3])) into my project I get [] but in node.js when I do that I get [ 1, 2, 3 ].

dantman · 2017-04-24T08:29:39Z

I believe I have tracked down the underlying issue.

At the start of my code I import 'core-js/es6/symbol'; so that ES6 Symbol is available.

React Native includes its own Set polyfill in Libraries/vendor/core/Set.js
_shouldPolyfillES6Collection('Set') returns true so RN's Set polyfill is used
When the polyfill is initialized toIterator.ITERATOR_SYMBOL is '@@iterator' because Symbol does not exist
All of this happens before any application code runs or polyfills are required
During runtime if you require the symbol polyfill or any polyfill that requires the symbol polyfill
- Symbol actually exists and Symbol.iterator is defined
- The Array.from polyfill will then look for iterators defined using the value of Symbol.iterator instead of '@@iterator'
- Because RN's custom Set polyfill doesn't use the correct iterator symbol Array.from(new Set(...)) now always returns []

If I remove all code that leads to a Symbol polyfill being required, the bug goes away.

Workaround 1

Remove anything that depends on a Symbol polyfill (not a very reasonable workaround).

Workaround 2

After requiring something that uses a Symbol polyfill, use this to hack the correct iterator into RN's custom polyfill:

{
	// https://github.com/facebook/react-native/issues/11968
	// RN currently has a custom `Set` polyfill that is broken when you use an actual Symbol polyfill
	let toIterator = require('react-native/Libraries/vendor/core/toIterator');
	if ( toIterator.ITERATOR_SYMBOL !== Symbol.iterator && Set.prototype[toIterator.ITERATOR_SYMBOL] ) {
		Set.prototype[Symbol.iterator] = Set.prototype[toIterator.ITERATOR_SYMBOL];
	}
}

Short-term fix

BackAndroid (now BackHandler.android) doesn't actually have any reason to depend on fancy iterator behaviours. The line immediately after the ES6 [...arr] syntax is an ES3 style for(var i ...) loop. And the code is unnecessarily adding more low level loops (new Set(_backPressSubscriptions) clones the set, [...backPressSubscriptions] creates an array and uses an interator function to fill it with the values in the cloned set, .reverse() then reverses the order of the array).

A simple fix would be to replace this:

  var backPressSubscriptions = new Set(_backPressSubscriptions);
  var invokeDefault = true;
  var subscriptions = [...backPressSubscriptions].reverse();

with this:

  var invokeDefault = true;
  var subscriptions = [];
  _backPressSubscriptions.forEach(function(subscription) {
    subscriptions.unshift(subscription);
  });

(If you think unshift is sub-optimal I could equally use push (or pre-calculate the array size) and make the for loop run backwards)

Long-term fix

Long term this is going to be a general problem with this pattern of React Native hardcoding its own embedded polyfill and likely will result in other parts of React Native breaking – the short-term fix after all doesn't actually fix Set; Set, Map, any any other polyfill in RN implementing an iterator will still be broken.

React Native should either abandon low-level ES6 polyfills with conflict potential like Set and Map tweaking internal RN code so it does not rely on them or React Native should directly require core-js' set and map polyfills.

dslounge · 2017-04-28T20:10:52Z

haha I can't believe I just spent an entire day trying to figure out why the back button wasn't working. It's the issue @dantman described. I was importing the Symbol polyfill from core-js. Back button was working on chrome debug mode, not in the regular app.

edgesite · 2017-05-03T07:51:25Z

@dantman
Can you submit a pull request for the short-term fix since the long-term fix needs time to approach?

hramos · 2017-07-20T22:13:16Z

Hi there! This issue is being closed because it has been inactive for a while. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. Either way, we're automatically closing issues after a period of inactivity. Please do not take it personally!

If you think this issue should definitely remain open, please let us know. The following information is helpful when it comes to determining if the issue should be re-opened:

Does the issue still reproduce on the latest release candidate? Post a comment with the version you tested.
If so, is there any information missing from the bug report? Post a comment with all the information required by the issue template.
Is there a pull request that addresses this issue? Post a comment with the PR number so we can follow up.

If you would like to work on a patch to fix the issue, contributions are very welcome! Read through the contribution guide, and feel free to hop into #react-native if you need help planning your contribution.

This usage 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. Fixes facebook#11968

dantman · 2017-07-25T01:04:23Z

I've added a PR

This usage 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. Fixes facebook#11968

This usage 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 use of `set.values()` which does not require the set's implicit iterator. Fixes facebook#11968

Summary: 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. 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 Closes #15182 Differential Revision: D6114696 Pulled By: hramos fbshipit-source-id: 2eae127558124a394bb3572a6381a5985ebf9d64

cosmith · 2018-01-18T17:12:50Z

For anyone still having this issue, since the commit 165b38a didn't actually fix it, you can use the fix by dantman by creating a new file where BackHandler doesn't use [...] or Set.values:

backhandler-custom.js

/**
 * Copyright (c) 2015-present, Facebook, Inc.
 * All rights reserved.
 *
 * This source code is licensed under the BSD-style license found in the
 * LICENSE file in the root directory of this source tree. An additional grant
 * of patent rights can be found in the PATENTS file in the same directory.
 *
 */

"use strict";

var DeviceEventManager = require("NativeModules").DeviceEventManager;
var RCTDeviceEventEmitter = require("RCTDeviceEventEmitter");

var DEVICE_BACK_EVENT = "hardwareBackPress";

type BackPressEventName = $Enum<{
    backPress: string,
}>;

var _backPressSubscriptions = new Set();

RCTDeviceEventEmitter.removeAllListeners(DEVICE_BACK_EVENT);

RCTDeviceEventEmitter.addListener(DEVICE_BACK_EVENT, function() {
    var invokeDefault = true;
    var subscriptions = [];
    _backPressSubscriptions.forEach(sub => subscriptions.unshift(sub));

    for (var i = 0; i < subscriptions.length; ++i) {
        if (subscriptions[i]()) {
            invokeDefault = false;
            break;
        }
    }

    if (invokeDefault) {
        BackHandler.exitApp();
    }
});

/**
 * Detect hardware button presses for back navigation.
 *
 * Android: Detect hardware back button presses, and programmatically invoke the default back button
 * functionality to exit the app if there are no listeners or if none of the listeners return true.
 *
 * tvOS: Detect presses of the menu button on the TV remote.  (Still to be implemented:
 * programmatically disable menu button handling
 * functionality to exit the app if there are no listeners or if none of the listeners return true.)
 *
 * iOS: Not applicable.
 *
 * The event subscriptions are called in reverse order (i.e. last registered subscription first),
 * and if one subscription returns true then subscriptions registered earlier will not be called.
 *
 * Example:
 *
 * ```javascript
 * BackHandler.addEventListener('hardwareBackPress', function() {
 *  // this.onMainScreen and this.goBack are just examples, you need to use your own implementation here
 *  // Typically you would use the navigator here to go to the last state.
 *
 *  if (!this.onMainScreen()) {
 *    this.goBack();
 *    return true;
 *  }
 *  return false;
 * });
 * ```
 */
var BackHandler = {
    exitApp: function() {
        DeviceEventManager.invokeDefaultBackPressHandler();
    },

    /**
   * Adds an event handler. Supported events:
   *
   * - `hardwareBackPress`: Fires when the Android hardware back button is pressed or when the
   * tvOS menu button is pressed.
   */
    addEventListener: function(
        eventName: BackPressEventName,
        handler: Function
    ): {remove: () => void} {
        _backPressSubscriptions.add(handler);
        return {
            remove: () => BackHandler.removeEventListener(eventName, handler),
        };
    },

    /**
   * Removes the event handler.
   */
    removeEventListener: function(eventName: BackPressEventName, handler: Function): void {
        _backPressSubscriptions.delete(handler);
    },
};

module.exports = BackHandler;

And then import BackHandler from "./backhandler-custom"; and everything should work.

AuroreM · 2018-01-30T09:25:09Z

Thanks, I've used this custom BackHandler =)

Nevertheless I needed to import it with a new name :
import BackAndroid from "./backhandler-custom";
Otherwise the one from react-native where still called

rohitgoyal · 2018-03-18T08:47:58Z

@cosmith Tried this. It still doesn't work for me.
Created backhandler-custom file and imported in my App.js

In my App.js added event listener for hardwareBackPress like this

...
import BackHandler from "./backhandler-custom"
componentDidMount(){
    BackHandler.addEventListener('hardwareBackPress', this.handleBackButton);
    ....
}

componentWillUnmount(){
    BackHandler.removeEventListener('hardwareBackPress', this.handleBackButton);
    ....
}
handleBackButton() {
    console.log('back button pressed')
    console.log('Actions.state',Actions.state)
    // return true;
    if (Actions.state.index === 0) {
      console.log('index 0')
      BackHandler.exitApp();
      return false
    }
    console.log('index ',Actions.state.index)
    Actions.pop()
    return true
  }

cosmith · 2018-03-19T09:59:47Z

@rohitgoyal maybe try importing it with another name like AuroreM

hramos added the Icebox label Jul 20, 2017

hramos closed this as completed Jul 20, 2017

dantman mentioned this issue Jul 25, 2017

Remove usage of the ... operator in BackHandler #15182

Closed

dantman mentioned this issue Sep 27, 2017

BackHandler and remote debug #15497

Closed

stubailo mentioned this issue Dec 18, 2017

Document that Symbol polyfill is required to use in React Native/old browsers ardatan/graphql-tools#535

Closed

facebook locked as resolved and limited conversation to collaborators Jul 20, 2018

react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Broken BackAndroid handling #11968

Broken BackAndroid handling #11968

rafales commented Jan 18, 2017

hramos commented Jan 18, 2017

dantman commented Apr 24, 2017

dantman commented Apr 24, 2017

dslounge commented Apr 28, 2017

edgesite commented May 3, 2017 •

edited

hramos commented Jul 20, 2017 •

edited

dantman commented Jul 25, 2017

cosmith commented Jan 18, 2018

AuroreM commented Jan 30, 2018

rohitgoyal commented Mar 18, 2018

cosmith commented Mar 19, 2018

Broken BackAndroid handling #11968

Broken BackAndroid handling #11968

Comments

rafales commented Jan 18, 2017

Description

Reproduction

Additional Information

hramos commented Jan 18, 2017

dantman commented Apr 24, 2017

dantman commented Apr 24, 2017

Workaround 1

Workaround 2

Short-term fix

Long-term fix

dslounge commented Apr 28, 2017

edgesite commented May 3, 2017 • edited

hramos commented Jul 20, 2017 • edited

dantman commented Jul 25, 2017

cosmith commented Jan 18, 2018

AuroreM commented Jan 30, 2018

rohitgoyal commented Mar 18, 2018

cosmith commented Mar 19, 2018

edgesite commented May 3, 2017 •

edited

hramos commented Jul 20, 2017 •

edited