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

Packager: deep react-native field (also known as browser/resolve.alias) shim #9899

Closed
wants to merge 1 commit into from

Conversation

jacobrosenthal
Copy link
Contributor

Ive been maintaining a react native bluetooth package that attempts to bring over the extensive node bluetooth ecosystem from node to ios and android

Its been a longstanding request from many of us, sometimes with accompanying PR's sometimes without, to be able to use the react-native (browser elsewhere) field to shim packages.
facebookarchive/node-haste#46
mjohnston/react-native-webpack-server#75
#6253
https://productpains.com/post/react-native/packager-support-resolvealias-ala-webpack/
https://productpains.com/post/react-native/support-resolvealias-ala-webpack

Since then we got first level shims in the form of 'react-native' field in package.json. This attempts to store the root module and use its redirectRequire as a priority replacement before the current module's redirectRequire.

I know Ill need tests, and maybe even more implementation as I dont understand the cache/haste stuff yet but I haven't gotten any discussion in the discourse yet so Im hoping to spark some here.

Thanks in advance for any consideration or feedback

@ghost
Copy link

ghost commented Sep 14, 2016

By analyzing the blame information on this pull request, we identified @davidaurelio to be a potential reviewer.

@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 Sep 14, 2016
@jacobrosenthal jacobrosenthal changed the title push root module down into dependency resolve and prefer it over loca… Packager: deep react-native field (also known as browser/resolve.alias) shim Sep 15, 2016
@ghost ghost 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 Sep 15, 2016
@christopherdro
Copy link
Contributor

👍
Wow this looks great! Thanks for the detailed write up.

Pinging @davidaurelio once more

@ghost ghost 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 Sep 21, 2016
@christopherdro
Copy link
Contributor

@davidaurelio Any chance you take a look at this so we can try and get it merged in?

@facebook-github-bot 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 Oct 14, 2016
@facebook-github-bot
Copy link
Contributor

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @davidaurelio as a potential reviewer. Could you take a look please or cc someone with more context?

@facebook-github-bot 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 Oct 14, 2016
}
return modulePath;
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (name === modulePath) {
  return Promise.resolve(fromModule.getPackage()).then(resolveRoot);
}
return name;

@@ -305,21 +307,38 @@ class ResolutionRequest {
});
}

_redirectRequire(fromModule, modulePath) {
return Promise.resolve(fromModule.getPackage()).then(p => {
_redirectRequire(fromModule, modulePath, rootModule) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rootModule could be optional?

@hramos
Copy link
Contributor

hramos commented Nov 10, 2016

Ping @jacobrosenthal, see feedback above.

@lacker
Copy link
Contributor

lacker commented Dec 1, 2016

Hey, it seems like this pull request has been abandoned. I am going to close this but if you are still working on it please feel free to reopen! I might be pulling the trigger a bit hastily but, well, let me know.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants