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

Adds packager configuration to support windows platform #7639

Closed
wants to merge 2 commits into from
Closed

Adds packager configuration to support windows platform #7639

wants to merge 2 commits into from

Conversation

rozele
Copy link
Contributor

@rozele rozele commented May 19, 2016

This pull request is a prerequisite to enabling the react-native-windows platform extension.

In the Resolver component, we need to add 'windows' to the list of platforms that are allowed in the DependencyGraph. We also need to add 'react-native-windows' (the name of the Windows platform extension NPM module) to the providesModuleNodeModules option. This allows the node_module folder check in the DependencyGraphHelper from node-haste to be bypassed for *.windows.js files in the Windows NPM package.

For good measure, I also included a change to blacklist.js to ensure .windows.js files are ignored when the packager is parameterized on a platform.

In the Resolver component, we need to add 'windows' to the list of platforms that are allowed in the dependency graph.  We also need to add 'react-native-windows' (the name of the Windows platform extension NPM module) to the `providesModuleNodeModules` option. This allows the node_module folder check in the DependencyGraphHelper from node-haste to be bypassed for *.windows.js files in the Windows NPM package.

For good measure, I also included a change to blacklist.js to ensure .windows.js files are ignored when the packager is parameterized on a platform.
@ghost
Copy link

ghost commented May 19, 2016

By analyzing the blame information on this pull request, we identified @davidaurelio and @cpojer to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels May 19, 2016
@bestander
Copy link
Contributor

Looks reasonable

@bestander
Copy link
Contributor

Really looking forward to try that on my Surface Pro 👍

@ide
Copy link
Contributor

ide commented May 19, 2016

Mostly looks good to me. Generally we don't want to add entries to providesModuleNodeModules but it probably makes sense to make an exception solely for RN Windows. The thing we don't want is for this array to grow into "RN for [every platform]". cc @vjeux

@rozele
Copy link
Contributor Author

rozele commented May 19, 2016

I'm also open to any re-architecture suggestions @ide. I think it would be great to see a configuration based approach, where the end user specifies which platforms are enabled (both here and in my node-haste PR: facebookarchive/node-haste#70) and which node_modules directories are allowed in haste. Eliminating as much of this static config as possible.

@ide
Copy link
Contributor

ide commented May 19, 2016

Yep I think that's the plan. But RN Windows is the one project that we might want to make a temporary exception for.

@mkonicek
Copy link
Contributor

Tests passed, let's shipit :)

@facebook-github-bot shipit

@ghost ghost added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels May 20, 2016
@ghost
Copy link

ghost commented May 20, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in ef21d99 May 20, 2016
@davidaurelio
Copy link
Contributor

providesModuleNodeModules is going away.

but it probably makes sense to make an exception solely for RN Windows.

and every other platform coming along? Can we come up with better ideas, please?

@mkonicek
Copy link
Contributor

mkonicek commented May 20, 2016

Just talked to @davidaurelio - we should eventually remove providesModuleNodeModules from the packager completely, have to come up with the right approach. For now we can keep this PR.

@davidaurelio
Copy link
Contributor

davidaurelio commented May 20, 2016

@ok, I tried to come up with something that might scale better:

what if the packager reads react-native-<platform>/package.json, and we read a mapping from there? "react-native" is already taken. Let’s assume "react-native-override".

It could look like this:

package.json

{ "name": "react-native-windows",
  "react-native-override": {
    "View": "lib/View.js",
    "Text": "lib/Text.js" } }

@rozele
Copy link
Contributor Author

rozele commented May 20, 2016

I presume there is some implied convention here that the DependencyGraph would use the part of react-native-<platform> as the module map platform key? What if a particular package contained multiple platform implementations for whatever reason? Maybe it just needs to be more verbose...

{ 
  "name" : "react-native-windows",
  "react-native-overrides" : {
    "windows" : {
      "Text": "lib/Text.windows.js"
    },
    "foo" : {
      "Text": "lib/Text.foo.js"
    }
  }
}

@davidaurelio
Copy link
Contributor

What if a particular package contained multiple platform implementations for whatever reason?

Do you have a specific scenario in mind? I’d like to keep it simple/separated, but maybe that’s to simplistic?

@ide
Copy link
Contributor

ide commented May 20, 2016

and every other platform coming along?

No, only Microsoft. We've rejected at least one other PR of this nature in the past. I just think that with the effort behind RN Windows and the fact that it's the primary focus of a couple engineers, it's the one time we should make a (temporary) exception.

@davidaurelio
Copy link
Contributor

I just have mixed feelings, because making this exceptions means it’s going to stay. I wanted to kill that feature in the next weeks, but that’s not going to happen now.

@rozele
Copy link
Contributor Author

rozele commented May 20, 2016

@davidaurelio, totally agree that this is not the cleanest approach, and I'd also like to see something better. It would be great for someone with much more experience with the packager to put a PR together, this was just the quickest way to unblock publishing a windows package for now, and it didn't seem much different from the 'web' platform stuff that's sprinkled throughout the packager but doesn't have any local backing code.

I think this change can and should definitely be temporary. Don't feel blocked on removing the providesModuleNodeModules feature at will, so long as an alternative for overriding modules from other packages is added in simultaneously ;-).

@rozele rozele deleted the windows branch May 23, 2016 18:53
zebulgar pushed a commit to nightingale/react-native that referenced this pull request Jun 18, 2016
Summary:
This pull request is a prerequisite to enabling the react-native-windows platform extension.

In the Resolver component, we need to add 'windows' to the list of platforms that are allowed in the DependencyGraph.  We also need to add 'react-native-windows' (the name of the Windows platform extension NPM module) to the `providesModuleNodeModules` option. This allows the node_module folder check in the DependencyGraphHelper from node-haste to be bypassed for *.windows.js files in the Windows NPM package.

For good measure, I also included a change to blacklist.js to ensure .windows.js files are ignored when the packager is parameterized on a platform.
Closes facebook#7639

Differential Revision: D3327771

Pulled By: mkonicek

fbshipit-source-id: d1080b045ff6aa0cbf05d8070ceb0eb4cdb6dceb
samerce pushed a commit to iodine/react-native that referenced this pull request Aug 23, 2016
Summary:
This pull request is a prerequisite to enabling the react-native-windows platform extension.

In the Resolver component, we need to add 'windows' to the list of platforms that are allowed in the DependencyGraph.  We also need to add 'react-native-windows' (the name of the Windows platform extension NPM module) to the `providesModuleNodeModules` option. This allows the node_module folder check in the DependencyGraphHelper from node-haste to be bypassed for *.windows.js files in the Windows NPM package.

For good measure, I also included a change to blacklist.js to ensure .windows.js files are ignored when the packager is parameterized on a platform.
Closes facebook#7639

Differential Revision: D3327771

Pulled By: mkonicek

fbshipit-source-id: d1080b045ff6aa0cbf05d8070ceb0eb4cdb6dceb
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
Summary:
This pull request is a prerequisite to enabling the react-native-windows platform extension.

In the Resolver component, we need to add 'windows' to the list of platforms that are allowed in the DependencyGraph.  We also need to add 'react-native-windows' (the name of the Windows platform extension NPM module) to the `providesModuleNodeModules` option. This allows the node_module folder check in the DependencyGraphHelper from node-haste to be bypassed for *.windows.js files in the Windows NPM package.

For good measure, I also included a change to blacklist.js to ensure .windows.js files are ignored when the packager is parameterized on a platform.
Closes facebook#7639

Differential Revision: D3327771

Pulled By: mkonicek

fbshipit-source-id: d1080b045ff6aa0cbf05d8070ceb0eb4cdb6dceb
cpojer pushed a commit to facebook/metro that referenced this pull request Jan 26, 2017
Summary:
This pull request is a prerequisite to enabling the react-native-windows platform extension.

In the Resolver component, we need to add 'windows' to the list of platforms that are allowed in the DependencyGraph.  We also need to add 'react-native-windows' (the name of the Windows platform extension NPM module) to the `providesModuleNodeModules` option. This allows the node_module folder check in the DependencyGraphHelper from node-haste to be bypassed for *.windows.js files in the Windows NPM package.

For good measure, I also included a change to blacklist.js to ensure .windows.js files are ignored when the packager is parameterized on a platform.
Closes facebook/react-native#7639

Differential Revision: D3327771

Pulled By: mkonicek

fbshipit-source-id: d1080b045ff6aa0cbf05d8070ceb0eb4cdb6dceb
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants