Fix regression in dynamic resolution for out-of-tree platforms #20662

matthargett · 2018-08-14T01:57:45Z

A recent Facebook commit broke dynamic resolution for out-of-tree platforms:
d5e9e55#diff-9b25f85c1d5984674020a6d0c7a66be9

The request for a fix resulted in another commit which hard-coded platforms:
c4bcca6#diff-9b25f85c1d5984674020a6d0c7a66be9

This PR restores the original functionality. I will update this PR with more tests before requesting merge.

Thanks to my colleague @rdy for pairing with me.

Test Plan:

[x] Ran yarn test
[ ] Add new e2e CLI tests to verify out-of-tree platforms

Release Notes:

[ GENERAL ] [ BUGFIX ] [local-cli ] Restore dynamic resolution for out-of-tree platforms.

…latform plugins. Unrelated style auto-fixes to get lint and prettier to pass. Next step is to add more tests that illustrate further regressions for this functionality.

pull-bot · 2018-08-14T02:00:00Z

	Warnings
⚠️	📋 Release Notes - This PR may have incorrectly formatted Release Notes.

Generated by 🚫 dangerJS

kelset · 2018-08-14T08:24:31Z

It looks like you had some prettier/linter changing files around - would you mind reversing those to leave only the "actual" changes needed (which I guess are the ones on jest/hasteImpl.js and local-cli/core/index.js and)?

Also, it looks like some tests are failing.

…amic-platforms

…c-platforms

kelset · 2018-08-17T08:21:06Z

LGTM now, but yeah we should get someone from FB to approve this.

rozele · 2018-08-17T15:41:50Z

jest/hasteImpl.js

@@ -37,7 +37,7 @@ const NAME_REDUCERS /*: Array<[RegExp, string]> */ = [
  // strip .js/.js.flow suffix
  [/^(.*)\.js(\.flow)?$/, '$1'],
  // strip .android/.ios/.native/.web suffix
-  [/^(.*)\.(android|ios|native|web|windows|dom)$/, '$1'],
+  [/^[^\.]+(\.es\d)?$/, '$1'],


Seems like this could use a comment, not sure how it works

I ran a test on the regex, and I can't get it to match Foo.native.js. This is the code that I had commented in Slack about. Unfortunately, hasteImpl is dynamically required by jest-haste-map, so it's not trivial to inject "plugin" platform extensions.

However, we can move to a model where plugin platforms must create their own metro.config.js and hasteImpl.js file with the platform extensions in the regex for getHasteName, but that would mean it would be more challenging to add plugin platforms to projects that already have their own metro.config.js. We could probably use babel to modify existing metro.config.js files and add hasteImplModulePath if it's not already there, and just throw a warning if it's already there pointing to instructions on how to add the necessary platform behavior to their hasteImpl.js.

It looks like this regex aims to strip out .es6 suffixes, but it needs one more capture group. As it stands right now, it would take index.es6 and return .es6, which is unlike the other reducers that strip out .js or .flow.js.

It looks like you want to use ^([^\.]+)(\.es\d)?$ here, which would return index in the above example.

This doesn't match the comment from the line above it, however.

hramos

Thanks for sending the fix. I'd also appreciate a comment on the platform regex, before merging this.

hramos

Pending clarification on regex's intent.

hramos · 2018-08-17T19:05:39Z

jest/hasteImpl.js

@@ -37,7 +37,7 @@ const NAME_REDUCERS /*: Array<[RegExp, string]> */ = [
  // strip .js/.js.flow suffix
  [/^(.*)\.js(\.flow)?$/, '$1'],
  // strip .android/.ios/.native/.web suffix
-  [/^(.*)\.(android|ios|native|web|windows|dom)$/, '$1'],
+  [/^[^\.]+(\.es\d)?$/, '$1'],


It looks like this regex aims to strip out .es6 suffixes, but it needs one more capture group. As it stands right now, it would take index.es6 and return .es6, which is unlike the other reducers that strip out .js or .flow.js.

It looks like you want to use ^([^\.]+)(\.es\d)?$ here, which would return index in the above example.

This doesn't match the comment from the line above it, however.

…re in the getPlatforms() list.

rozele · 2018-08-17T21:44:42Z

I'm not convinced about the regex change. My concern is that this forces the repo to stick to a strict convention that module.X.js is only used for X = platform, when no such strict requirement existed before, as the set of platform moniker was enumerated. I agree that something needs to be done to address this for extracting haste names for other platforms, but I'm not positive this is the best solution.

matthargett · 2018-08-17T22:33:16Z

Yea, as I'm iterating and seeing the issues unfold, it turned out that assuming the convention would hold in a consistent way was a poor choice on my part. I'm open to suggestions that can be implemented in time for cherry-picking into the next 0.57 RC.

…prevents us from adding a file to the repo for out-of-tree platforms that allows us to resolve Haste modules at the top-level of the repo. This change ensures that if a user has a file, those settings are not overwritten by React Native.

…c-platforms

…platforms

…c-platforms

matthargett · 2018-08-23T20:26:50Z

@empyrical @rozele @CaptainNic @madhavarshney @Maxris @vincentriemer If you could check if this approach of overriding via metro.config.js works for your projects, it would be appreciated. The plan is for this to be cherry-picked into 0.57. Thanks!

empyrical · 2018-08-23T21:17:50Z

local-cli/core/index.js

  },

  getProvidesModuleNodeModules(): Array<string> {
-    return ['react-native', 'react-native-windows', 'react-native-dom'];
+    return ['react-native', ...plugins.platforms];


plugins.platforms is an array of strings like this:

[ 'react-native-foo/local-cli/platform.js' ]

Which it gets from an entry in react-native-foo's package.json like this:

"rnpm": { "platform": "./local-cli/platform.js" }

But getProvidesModuleNodeModules() just wants entries like react-native-foo, so unless I did something wrong this doesn't seem to work.

BTW, I just discovered this repo:

https://github.com/react-native-community/discussions-and-proposals/issues

Perhaps a discussion on how renderer plugins could be implemented could be started there too?

Fixed. Yes, that would be a good place to propose a new API, or discuss on the contributor's slack.

… the problem reported.

empyrical · 2018-08-24T00:01:31Z

I made a version partially inspired by how you did it, and also inspired by the "jest": { "haste": ... } entries in RN's `package.json.

empyrical@d7a0c6b
(edit: diff commit url)

You add this entry to your react-native-foo renderer's package.json:

{
  "rnpm": {
    "haste": {
      "providesModuleNodeModules": [
        "react-native-foo"
      ],
      "platforms": [
        "foo"
      ]
    }
}

And both jest/hasteImpl.js and local-cli/core/index.js will look for react native plugins that have this haste entry.

One thing I find appealing about this way is it leaves open the possibility to declare multiple providesModuleNodeModules and platforms for the packager to look in, if you want. And could be extended to have more Metro configuration options later on too. If you're okay with me doing this, perhaps I could submit this commit as a pull request for consideration too? (I want to clean some things up first and test it a bit more, though)

empyrical · 2018-08-24T02:03:04Z

Made a proposal here: react-native-community/discussions-and-proposals#21

matthargett · 2018-08-24T18:28:16Z

Closing in favor of #20825

Summary: This pull request adds the ability for a platform developer to provide a `"haste"` key under the `"rnpm"` key in their `package.json` which allows the packager to pick up that platform's javascript files. The intent is to remove the need to have custom platforms hardcoded in. This is inspired by the `"jest": { "haste": {} }` key used by jest. For example, React Native Dom would have an entry like: ```json { "rnpm": { "haste": { "providesModuleNodeModules": [ "react-native-dom" ], "platforms": [ "dom" ] } } } ``` Support for more keys (path blacklists perhaps?) could be added in the future. This succeeds #20662, as per a discussion I had with matthargett. I've got an open discussion over here as well: react-native-community/discussions-and-proposals#21 Pull Request resolved: #20825 Differential Revision: D9596429 Pulled By: hramos fbshipit-source-id: a02f0da0bea8870bdc45d55e23da8ccbc36249f2

Summary: This pull request adds the ability for a platform developer to provide a `"haste"` key under the `"rnpm"` key in their `package.json` which allows the packager to pick up that platform's javascript files. The intent is to remove the need to have custom platforms hardcoded in. This is inspired by the `"jest": { "haste": {} }` key used by jest. For example, React Native Dom would have an entry like: ```json { "rnpm": { "haste": { "providesModuleNodeModules": [ "react-native-dom" ], "platforms": [ "dom" ] } } } ``` Support for more keys (path blacklists perhaps?) could be added in the future. This succeeds facebook#20662, as per a discussion I had with matthargett. I've got an open discussion over here as well: react-native-community/discussions-and-proposals#21 Pull Request resolved: facebook#20825 Differential Revision: D9596429 Pulled By: hramos fbshipit-source-id: a02f0da0bea8870bdc45d55e23da8ccbc36249f2

Initial changes to make platform resolution dynamic for out-of-tree p…

d314ef8

…latform plugins. Unrelated style auto-fixes to get lint and prettier to pass. Next step is to add more tests that illustrate further regressions for this functionality.

matthargett requested review from grabbou and Kureev as code owners August 14, 2018 01:57

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 14, 2018

react-native-bot added Impact: Regression Describes a behavior that used to work on a prior release, but stopped working recently. Core Team Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Aug 14, 2018

matthargett added 5 commits August 15, 2018 14:00

Merge branch 'master' of github.com:matthargett/react-native into dyn…

94425cc

…amic-platforms

Revert fixes for lint/prettier failures.

a5190b7

Merge branch 'master' of github.com:facebook/react-native into dynami…

a51bffe

…c-platforms

Take Name.es6.js and Name.es7.js into account for regexp.

771df00

Revert prettier auto-fix.

403930c

kelset mentioned this pull request Aug 17, 2018

0.57.0 Discussion react-native-community/releases#34

Closed

rozele reviewed Aug 17, 2018

View reviewed changes

hramos approved these changes Aug 17, 2018

View reviewed changes

hramos suggested changes Aug 17, 2018

View reviewed changes

Adjust comment and regexp. Note weird error when only two platforms a…

1f7f362

…re in the getPlatforms() list.

react-native-bot added the ‼️Large PR label Aug 17, 2018

matthargett added 3 commits August 21, 2018 18:16

Merge branch 'master' of github.com:facebook/react-native into dynami…

9edfa98

…c-platforms

Remove hard-coded platform paths.

5207cd1

kelset removed ‼️Large PR Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Aug 23, 2018

hasteImpl will be overridden via metro configurations of out-of-tree …

0e72049

…platforms

matthargett added 3 commits August 23, 2018 12:48

Merge branch 'master' of github.com:facebook/react-native into dynami…

3159e0c

…c-platforms

Update tests to remove out-of-tree platform specifics

d366ea4

Fix comment

9ce4e7a

empyrical reviewed Aug 23, 2018

View reviewed changes

My synthetic tests didn't capture real-world pathing. This should fix…

5733fe1

… the problem reported.

empyrical mentioned this pull request Aug 24, 2018

Supporting Third-party Platforms in RNPM + Metro Bundler react-native-community/discussions-and-proposals#21

Closed

empyrical mentioned this pull request Aug 24, 2018

Add support for out-of-tree platform plugins #20825

Closed

matthargett closed this Aug 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix regression in dynamic resolution for out-of-tree platforms #20662

Fix regression in dynamic resolution for out-of-tree platforms #20662

matthargett commented Aug 14, 2018

pull-bot commented Aug 14, 2018 •

edited

Loading

kelset commented Aug 14, 2018 •

edited

Loading

kelset commented Aug 17, 2018

rozele Aug 17, 2018

rozele Aug 17, 2018 •

edited

Loading

hramos Aug 17, 2018 •

edited

Loading

hramos left a comment

hramos left a comment

hramos Aug 17, 2018 •

edited

Loading

rozele commented Aug 17, 2018

matthargett commented Aug 17, 2018

matthargett commented Aug 23, 2018

empyrical Aug 23, 2018

matthargett Aug 23, 2018

empyrical commented Aug 24, 2018 •

edited

Loading

empyrical commented Aug 24, 2018

matthargett commented Aug 24, 2018

Fix regression in dynamic resolution for out-of-tree platforms #20662

Fix regression in dynamic resolution for out-of-tree platforms #20662

Conversation

matthargett commented Aug 14, 2018

Test Plan:

Release Notes:

pull-bot commented Aug 14, 2018 • edited Loading

kelset commented Aug 14, 2018 • edited Loading

kelset commented Aug 17, 2018

rozele Aug 17, 2018

Choose a reason for hiding this comment

rozele Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

hramos Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

hramos left a comment

Choose a reason for hiding this comment

hramos left a comment

Choose a reason for hiding this comment

hramos Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

rozele commented Aug 17, 2018

matthargett commented Aug 17, 2018

matthargett commented Aug 23, 2018

empyrical Aug 23, 2018

Choose a reason for hiding this comment

matthargett Aug 23, 2018

Choose a reason for hiding this comment

empyrical commented Aug 24, 2018 • edited Loading

empyrical commented Aug 24, 2018

matthargett commented Aug 24, 2018

pull-bot commented Aug 14, 2018 •

edited

Loading

kelset commented Aug 14, 2018 •

edited

Loading

rozele Aug 17, 2018 •

edited

Loading

hramos Aug 17, 2018 •

edited

Loading

hramos Aug 17, 2018 •

edited

Loading

empyrical commented Aug 24, 2018 •

edited

Loading