Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue with bundler while using Platform.select in release build #17852

Closed
varungupta85 opened this issue Feb 4, 2018 · 7 comments
Closed

Issue with bundler while using Platform.select in release build #17852

varungupta85 opened this issue Feb 4, 2018 · 7 comments
Labels
Ran Commands One of our bots successfully processed a command. Resolution: Locked This issue was locked by the bot.

Comments

@varungupta85
Copy link
Contributor

varungupta85 commented Feb 4, 2018

Is this a bug report?

Yes

Have you read the Contributing Guidelines?

Yes

Environment

Environment:
OS: macOS Sierra 10.12.6
Node: 8.2.1
Yarn: 1.1.0
npm: 5.3.0
Watchman: 4.9.0
Xcode: Xcode 9.2 Build version 9C40b
Android Studio: 2.3 AI-162.4069837

Packages: (wanted => installed)
react: ^16.2.0 => 16.2.0
react-native: ^0.52.2 => 0.52.2

Target Platform: iOS and Android

Steps to Reproduce

I have a react-native app in production for the last few months. I updated to latest react-native 0.52.2 and started to see some strange problems with Platform.select behaviour only in the release app. Values set using Platform.select were not set in certain cases. I was able to confirm it by updating the release app to print logs. I want to emphasise again that it happens only in release app and NOT in development app. Below are some code excerpts that work and don't work.

// I have a reusable module to show a context menu.
// It exposes an API `ContextMenu.show(options)` that can be used to show the context menu.
// The items in the context menu could be different on iOS vs Android. 
// Below piece of code used to work before I updated to 0.52.2 but it doesn't work now.

let ctxOptions = {
  items: Platform.select({
    'ios': showDelete ? [I18n.t('takePhoto'), I18n.t('chooseFromLibrary'), I18n.t('delete'), I18n.t('cancel')] :
      [I18n.t('takePhoto'), I18n.t('chooseFromLibrary'), I18n.t('cancel')],
    'android': showDelete ? [I18n.t('takePhoto'), I18n.t('chooseFromLibrary'), I18n.t('delete')] :
      [I18n.t('takePhoto'), I18n.t('chooseFromLibrary')]
  }),
  info: Platform.select({
    'ios': {
      destructiveButtonIndex: showDelete ? 2 : undefined,
      cancelButtonIndex: showDelete ? 3 : 2
    },
    'android': {}
  })
}

// Both info and items are undefined on both iOS and Android
ContextMenu.show(ctxOptions)

// I changed the above code to not use Platform.select but just pick the right object based on Platform.OS value without changing anything else and this works

let ctxOptions = {
  items: ({
    'ios': showDelete ? [I18n.t('takePhoto'), I18n.t('chooseFromLibrary'), I18n.t('delete'), I18n.t('cancel')] :
      [I18n.t('takePhoto'), I18n.t('chooseFromLibrary'), I18n.t('cancel')],
    'android': showDelete ? [I18n.t('takePhoto'), I18n.t('chooseFromLibrary'), I18n.t('delete')] :
      [I18n.t('takePhoto'), I18n.t('chooseFromLibrary')]
  })[Platform.OS],
  info: ({
    'ios': {
      destructiveButtonIndex: showDelete ? 2 : undefined,
      cancelButtonIndex: showDelete ? 3 : 2
    },
    'android': {}
  })[Platform.OS]
}

// Both info and items are set according to the OS
ContextMenu.show(ctxOptions)

// I also changed the above statements to ternary statements using Platform.OS and that works as well.

I am using Platform.select at other places such as styles such as below where it works as expected.

const styles = StyleSheet.create({
  container: Platform.select({
    android: {
      flex: 1,
      justifyContent: 'center',
      alignItems: 'center',
      backgroundColor: 'rgba(0, 0, 0, 0.4)',
    },
    ios: {
      justifyContent: 'center',
      alignItems: 'center',
    },
  })
})

This leads me to believe that this may be an issue with the bundler used in the release app causing the values to be not set.

Expected Behavior

The values set using Platform.select should be set according to the OS

Actual Behavior

The values were not set as described above.

Reproducible Demo

I have created a new project which can be downloaded from here. In the App.js file, in componentDidMount, there are two calls: one with Platform.select and the other with Platform.OS as described above. The app will abort when Platform.select section is not commented out and wouldn't abort when Platform.OS section is not commented out only in the release builds.

@brunolemos
Copy link
Contributor

Hi @varungupta85, thanks for reporting!
I was able to reproduce the bug.

Here is a smaller version for reproduction:

const select1 = Platform.select({ ios: 'ios', default: 'default' })
const select2 = Platform.select({ 'ios': 'ios', default: 'default' })
const select3 = Platform.select({ 'ios': 'ios', 'default': 'default' })

alert(`OS: ${Platform.OS}, 1: ${select1}, 2: ${select2}, 3: ${select3}`)
// expected:          OS: ios, 1: ios, 2: ios, 3: ios
// result on release: OS: ios, 1: ios, 2: default, 3: undefined

As you can see, this bug is happening when you write the object keys using quotes.

As a workaround , please remove the quotes for now, while this issue is not fixed.

Thank you!

@brunolemos
Copy link
Contributor

After more investigation, it works fine on 0.51.0 and was introduced on 0.52.0.
Not sure which commit, but my guess would be this metro bundler version bump: 0bbd9f0

cc @rafeca

brunolemos referenced this issue Feb 4, 2018
Summary:
`metro-bundler` v0.21 contains a rewritten bundling mechanism, with simplified logic and much faster rebuild times, called delta bundler. This release contains a couple of breaking changes:

* Now, when using a custom transformer, the list of additional babel plugins to apply are passed to the `transform()` method. These are used in non-dev mode for optimization purposes (Check facebook/metro@367a5f5#diff-40653f0c822ac59a5af13d5b4ab31d84 to see how to handle them from the transformer).
* Now, when using a custom transformer outputting `rawMappings`, the transformer does not need to call the `compactMappings` method before returning (check facebook/metro@d74685f#diff-40653f0c822ac59a5af13d5b4ab31d84 for more info).
* We've removed support for two config parameters: `postProcessModules` and `postProcessBundleSourcemap`.

Reviewed By: davidaurelio

Differential Revision: D6186035

fbshipit-source-id: 242c5c2a954c6b9b6f339d345f888eaa44704579
@varungupta85
Copy link
Contributor Author

@brunolemos Thanks for investigating the issue further and wow, I didn't realise that the quotes in the key names could have caused this. I guess, I shouldn't take anything for granted 😃 Thanks for the workaround. I will check other places in my code as well where I may be using quotes in the key names.

@varungupta85
Copy link
Contributor Author

varungupta85 commented Feb 5, 2018

I am a little confused by where does this problem affect and where it doesn't. For e.g., I have an object defined as below:

    CascadingAlarmIntervals: {
      '5mins': {
        label: I18n.t('minutes', {count: 5}),
        value: 300000
      },
      '10mins': {
        label: I18n.t('minutes', {count: 10}),
        value: 600000
      },
      '15mins': {
        label: I18n.t('minutes', {count: 15}),
        value: 900000
      },
      '30mins': {
        label: I18n.t('minutes', {count: 30}),
        value: 1800000
      },
      '1hr': {
        label: I18n.t('hours', {count: 1}),
        value: 3600000
      },
      '2hr': {
        label: I18n.t('hours', {count: 2}),
        value: 7200000
      },
      '3hr': {
        label: I18n.t('hours', {count: 3}),
        value: 10800000
      }
    }

This works fine even though the key names have strings. I can't change the key names to be not strings because they start with a number and eslint throws an error.

@rafeca
Copy link
Contributor

rafeca commented Feb 5, 2018

This could have been caused by one of the changes in metro. At this point it's hard for me to bisect the actual metro change since lots of things changed between the metro version in RN 0.51 and 0.52 (we almost rebuilt the bundling logic).

If anybody is up to fix it, the logic that needs to be updated is the one in inline-platform.js, which transform the Platform.select() calls in prod to remove the non-used platforms (so if you have Platform.select({ios: require('implementation-ios.js'), android: require('implementation-android.js')});, when you build the iOS app the android code won't be included in the bundle, and viceversa).

@varungupta85 : the last code you posted should work fine (the issue only affects Platform.select() calls).

@react-native-bot
Copy link
Collaborator

Thanks for posting this! It looks like you may not be using the latest version of React Native, v0.53.0, released on January 2018. Can you make sure this issue can still be reproduced in the latest version?

I am going to close this, but please feel free to open a new issue if you are able to confirm that this is still a problem in v0.53.0 or newer.

How to ContributeWhat to Expect from Maintainers

@react-native-bot react-native-bot added Ran Commands One of our bots successfully processed a command. Stale There has been a lack of activity on this issue and it may be closed soon. labels Feb 24, 2018
@stale stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Feb 24, 2018
@salmanwaheed

This comment has been minimized.

@facebook facebook locked as resolved and limited conversation to collaborators Feb 24, 2019
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Feb 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Ran Commands One of our bots successfully processed a command. Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

5 participants