Add support for out-of-tree platform plugins by empyrical · Pull Request #20825 · facebook/react-native

empyrical · 2018-08-24T07:26:12Z

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:

{
  "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

Test Plan:

I've modified a local copy of RNDom to have the above example in its package.json in an unmodified default React Native app project, and a JS bundle was build successfully.

Release Notes:

[CLI] [ENHANCEMENT] [local-cli/core/findPlugins.js] - Modified the findPlugins function to handle this new haste field
[CLI] [ENHANCEMENT] [local-cli/core/index.js] - Removed the hardcoded platforms and switched to use output from findPlugins

eslint-bot · 2018-08-24T07:28:24Z

local-cli/core/findPlugins.js

@@ -71,11 +98,12 @@ const findPluginInFolder = folder => {
        if (pkgJson) {
          commands = commands.concat(findPluginsInReactNativePackage(pkgJson));


prettier/prettier: Delete ··

I am not entirely sure what this means

eslint-bot · 2018-08-24T07:28:24Z

jest/hasteImpl.js

-  [/^(.*)\.(android|ios|native|web|windows|dom)$/, '$1'],
+  // strip platform suffix
+  [/^(.*)\.(android|ios|native)$/, '$1'],
+  // strip plugin platform suffixes


prettier/prettier: Insert ,

empyrical · 2018-08-24T07:32:00Z

@rozele: I had to change around the lines you modified in #20705 because it seems like config.resolver.platforms was always present with metro defaults, meaning defaultConfig.getPlatforms() was never reached even without a metro.config.js. I still have the undefined check there because I am not sure if there are still cases where it will be empty. I am not sure if this will break your use case or not.

eslint-bot · 2018-08-24T07:53:15Z

local-cli/core/findPlugins.js

+  }
+};
+
 const findPluginInFolder = folder => {


prettier/prettier: Replace flatten(plugins.map(p·=>·p.haste.providesModuleNodeModules)) with ⏎········flatten(plugins.map(p·=>·p.haste.providesModuleNodeModules)),⏎······

matthargett · 2018-08-24T18:27:03Z

@rozele @vincentriemer @CaptainNic I’m closing #20662 in favor of this approach.

eslint-bot · 2018-08-25T00:26:03Z

RNTester/js/ImageExample.js

-            }]}
+            style={[
+              styles.base,
+              styles.leftMargin,


react-native/no-inline-styles: Inline style: { borderWidth: 5,
borderTopLeftRadius: 10,
borderTopRightRadius: 20,
borderBottomRightRadius: 30,
borderBottomLeftRadius: 40,
borderColor: 'red' }

eslint-bot · 2018-08-25T00:26:04Z

RNTester/js/OutOfTreeTestPlatform/Libraries/Components/StatusBar/StatusBarIOS.dummy.js

+
+const NativeEventEmitter = require('NativeEventEmitter');
+
+module.exports = new NativeEventEmitter('StatusBarManager');


Cannot call NativeEventEmitter with 'StatusBarManager' bound to nativeModule because string [1] is incompatible with NativeModule [2].

eslint-bot · 2018-08-25T00:48:46Z

RNTester/js/ImageExample.js

-            }]}
+            style={[
+              styles.base,
+              styles.leftMargin,


react-native/no-inline-styles: Inline style: { borderWidth: 5,
borderTopLeftRadius: 10,
borderTopRightRadius: 20,
borderBottomRightRadius: 30,
borderBottomLeftRadius: 40,
borderColor: 'red' }

empyrical · 2018-08-25T01:10:02Z

I've added a dummy platform in RNTester/js/OutOfTreeTestPlatform with the aim of testing RNCLI's support for bundling out of tree platforms.

Currently, there is just a test in ContainerShip/scripts/run-ci-e2e-tests.sh - I wanted to also have a platform suffix test in jest/__tests__/hasteImpl-test.js, but I need to be able to have OutOfTreeTestPlatform/react-native-dummy as a devDependency somehow for that to work. I do not predict this dummy module needing to be changed much - perhaps it could just be installed from npm, and added as another package that gets published in the publish script?

I currently have this module on NPM for testing purposes that I could transfer ownership over if FB devs are interested:

https://www.npmjs.com/package/react-native-dummy

empyrical · 2018-08-26T18:19:54Z

I've added an entry to package.json that installs this local module using this syntax:

    "react-native-dummy": "file:./RNTester/js/OutOfTreeTestPlatform",

Which was sufficient for letting me get a hasteImpl test working.

matthargett

Mostly questions and minor styling. Fantastic work on adding unit and e2e tests at the same time! 🥇

matthargett · 2018-08-27T23:04:36Z

RNTester/js/OutOfTreeTestPlatform/Libraries/Components/ProgressViewIOS/ProgressViewIOS.dummy.js

+
+class DummyProgressViewIOS extends React.Component {
+  render() {
+    return (


why not just have the render throw("Not implemented") or something similar? or just export require('UnimplementedView'); like the other files?

matthargett · 2018-08-27T23:04:55Z

.../OutOfTreeTestPlatform/Libraries/Components/SegmentedControlIOS/SegmentedControlIOS.dummy.js

+class DummySegmentedControlIOS extends React.Component {
+  render() {
+    return (
+      <View style={[styles.dummy, this.props.style]}>


same question here -- why not just throw?

matthargett · 2018-08-27T23:05:46Z

RNTester/js/OutOfTreeTestPlatform/Libraries/Components/StatusBar/StatusBarIOS.dummy.js

+
+'use strict';
+
+module.exports = {};


why empty here and not require('UnimplementedView'); as elsewhere?

StatusBarIOS isn't a React component

https://github.com/facebook/react-native/blob/master/Libraries/Components/StatusBar/StatusBarIOS.ios.js

None if this code will be ran at all at any point - it is purely for the bundler's sake, so the actual code isn't that important. A lot of the shims, like DummySegmentedControlIOS are just copied over from Android shims in the RN codebase. Sorry for not making this clearer! I'll edit the main post

matthargett · 2018-08-27T23:07:36Z

RNTester/js/OutOfTreeTestPlatform/Libraries/Settings/Settings.dummy.js

+'use strict';
+
+// Completely empty shim for the sake of bundling.
+const Settings = {};


if require('UnimplementedView'); can't work here, let's inline the assignment as elsewhere.

empyrical · 2018-08-28T02:50:43Z

I've run unit tests on my fork on a branch that's been rebased against #20854, and it turns out that jest-haste-map doesn't like two instances of this module being in its search paths at the same time, and it still seems to see it despite me adding it to all search path blacklists.

jest-haste-map: @providesModule naming collision:
  Duplicate module name: react-native-dummy
  Paths: /home/circleci/react-native/node_modules/react-native-dummy/package.json collides with /home/circleci/react-native/RNTester/js/OutOfTreeTestPlatform/package.json

This error is caused by a @providesModule declaration with the same name across two different files.

I honestly can't think of a way to get around this, so I will be trying this again as a separate pull request. If anyone wants to peek around at the code, it still exists at this branch:

https://github.com/empyrical/react-native/tree/haste-plugins-plus-test

jhampton · 2018-08-28T05:43:39Z

@empyrical I think have run across this before. I was trying to do module substitution to emulate tree shaking for an out-of-tree platform. My approach used the getTransformModulePath option in the rn-cli.config.js (I haven't found a ton of documentation, so the signature may have changed). Basically this option is a path to a file that returns a function that lets you hook into the module resolution and decide to replace the implementation wholesale. I can provide a sample and basic walkthrough of what I was doing if you can DM me.

I'm by no means a Metro expert, but I've done a bit of hacking lately with it.

Cheers, and I'll take a look at your fork as well to see what I see.

-Jeff

kelset · 2018-08-28T09:34:57Z

@rozele @vincentriemer I'd love to hear your feedback on this, seems solid - and thanks for adding the tests too!

vincentriemer · 2018-08-28T14:10:27Z

I’ll try and take a look tonight 👌

vincentriemer · 2018-08-28T21:39:18Z

Short of actually pulling the branch and testing it against rndom itself, LGTM 👍

hramos

Test failures seem unrelated to this PR.

eslint-bot · 2018-08-30T17:17:13Z

local-cli/core/index.js

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


prettier/prettier: Replace ·config.resolver.providesModuleNodeModules·|| with ⏎····config.resolver.providesModuleNodeModules·||⏎···

eslint-bot · 2018-08-30T17:18:16Z

local-cli/core/index.js

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


prettier/prettier: Replace ·config.resolver.providesModuleNodeModules·|| with ⏎····config.resolver.providesModuleNodeModules·||⏎···

eslint-bot · 2018-08-30T17:18:16Z

local-cli/core/index.js


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


prettier/prettier: Insert ⏎···

empyrical · 2018-08-30T18:24:05Z

Rebased against master, and tests still look good. Looks like I may need to run prettify again (maybe the style guide settings were changed in one of the commits?)

facebook-github-bot

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

facebook-github-bot

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

react-native-bot · 2018-08-30T23:40:03Z

@empyrical merged commit 03476a2 into facebook:master.

Once this commit is added to a release, you will see the corresponding version tag below the description at 03476a2. If the commit has a single master tag, it is not yet part of a release.

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

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: facebook/react-native#20825 Differential Revision: D9596429 Pulled By: hramos fbshipit-source-id: a02f0da0bea8870bdc45d55e23da8ccbc36249f2

Taken from: facebook/react-native@03476a2 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: facebook/react-native#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

empyrical requested review from Kureev and grabbou as code owners August 24, 2018 07:26

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 24, 2018

eslint-bot reviewed Aug 24, 2018

View reviewed changes

react-native-bot added ✅Release Notes labels Aug 24, 2018

matthargett mentioned this pull request Aug 24, 2018

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

Closed

empyrical requested a review from hramos as a code owner August 25, 2018 00:24

eslint-bot reviewed Aug 25, 2018

View reviewed changes

empyrical force-pushed the haste-plugins-fixed branch from b5d5865 to e52ce92 Compare August 25, 2018 00:46

eslint-bot reviewed Aug 25, 2018

View reviewed changes

empyrical force-pushed the haste-plugins-fixed branch from e52ce92 to 92c1b7b Compare August 25, 2018 00:50

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

matthargett approved these changes Aug 27, 2018

View reviewed changes

empyrical force-pushed the haste-plugins-fixed branch from 52356b9 to 7c013d6 Compare August 28, 2018 02:37

hramos approved these changes Aug 29, 2018

View reviewed changes

empyrical force-pushed the haste-plugins-fixed branch from 2a83371 to 3a3bda4 Compare August 30, 2018 16:44

empyrical requested a review from janicduplessis as a code owner August 30, 2018 16:44

empyrical added 4 commits August 30, 2018 11:01

Add support for out-of-tree platform plugins

cd79e08

Fix JavaScript Style

09458fe

Correct hasteImpl to remove hardcoded platforms

6a76f55

Run prettier on platform support files

a73d991

empyrical force-pushed the haste-plugins-fixed branch from b0e7305 to a73d991 Compare August 30, 2018 17:02

eslint-bot reviewed Aug 30, 2018

View reviewed changes

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Aug 30, 2018

facebook-github-bot approved these changes Aug 30, 2018

View reviewed changes

facebook-github-bot closed this in 03476a2 Aug 30, 2018

facebook locked as resolved and limited conversation to collaborators Aug 30, 2018

react-native-bot added the Merged This PR has been merged. label Aug 30, 2018

hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 6, 2019

facebook-github-bot added the Contributor A React Native contributor. label Apr 15, 2024

		@@ -71,11 +98,12 @@ const findPluginInFolder = folder => {
		if (pkgJson) {
		commands = commands.concat(findPluginsInReactNativePackage(pkgJson));


		const NativeEventEmitter = require('NativeEventEmitter');

		module.exports = new NativeEventEmitter('StatusBarManager');

Conversation

empyrical commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Plan:

Release Notes:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

empyrical commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthargett commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

empyrical commented Aug 25, 2018

Uh oh!

empyrical commented Aug 26, 2018

Uh oh!

matthargett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

empyrical commented Aug 28, 2018

Uh oh!

jhampton commented Aug 28, 2018

Uh oh!

kelset commented Aug 28, 2018

Uh oh!

vincentriemer commented Aug 28, 2018

Uh oh!

vincentriemer commented Aug 28, 2018

Uh oh!

hramos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

empyrical commented Aug 30, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

react-native-bot commented Aug 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

empyrical commented Aug 24, 2018 •

edited

Loading