-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
support react-native field with fallback to browser field #2208
Conversation
I like this approach more than anything else I've seen so far. |
+1 |
Nice! We're in the process of moving the packager to it's own github repo and we'll be able to accept contributions like this much faster (we can just use the Github merge button). I'm hoping we can release it some time this week, and I'll ping you so we can move this PR there. Thanks! |
@amasad great, see you there :) |
@mvayngrib updated the pull request. |
Awesome, @mvayngrib sorry for the huge wait. Do you mind rebasing and i'll pull it in? Thanks! |
@vjeux will do after Thurs, under serious time pressure till then. If it's urgent, I can add u as a collaborator on my fork. |
Not urgent at all, it waited for a long time, can wait a bit more :)
|
This looks like a great fix, but I have a couple of questions. First, packager appears to support building web apps. I haven't used it for that and don't know if anyone else does, but if building for web, will it pick up the browser field instead of the react-native field from the package? Second, I wonder what folks who use webpack for building react-native will have to do to make use of this field? Perhaps there's a webpack config option? or maybe webpack will have to be modified... |
628362c
to
8d1c186
Compare
@mvayngrib updated the pull request. |
@@ -90,4 +91,8 @@ class Package { | |||
} | |||
} | |||
|
|||
function getReplacements (pkg) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: remove space between fn name and open paren.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May want to do a more robust check below that looks whether pkg[react-native] == null so that it returns empty strings when specified
8d1c186
to
357d6da
Compare
@@ -90,4 +91,10 @@ class Package { | |||
} | |||
} | |||
|
|||
function getReplacements(pkg) { | |||
return pkg['react-native'] == null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any plans to support excludes? (like "./lib/blah.js": false
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Browserify supports it and it's not too hard to add, we could support it eventually, if the need arises.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, with browserify it's pretty much a staple
@mvayngrib updated the pull request. |
@vjeux can you or @martinbigio import this then? |
Finally :) |
@facebook-github-bot shipit |
Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/842517002521880/int_phab to review. |
b86f147
Summary: React Native is a lot more powerful an environment than the browser, so we need an alternate mapping, as specified [here](https://github.com/defunctzombie/node-browser-resolve#browser-field) An example: ```js { "browser": { "./lib/server": false }, "react-native": { "dgram": "react-native-udp", "fs": "react-native-level-fs" }, "chromeapp": { "dgram": "chrome-dgram", "fs": "level-filesystem" } } ``` on the other hand, if "react-native" is not present in package.json, you should fall back to "browser" other than the one (nesting) test added, the tests are unchanged, just done for both "react-native" and "browser" (I've implemented [react-native-udp](https://npmjs.org/package/react-native-udp) and [react-native-level-fs](https://npmjs.org/package/react-native-level-fs), but they obviously don't belong in the traditional "browser" field as they won't run anywhere except in React Native.) Closes facebook#2208 Reviewed By: svcscm Differential Revision: D2691236 Pulled By: vjeux fb-gh-sync-id: 34041ed50bda4ec07f31d1dc50dcdfa428af2512
Summary: React Native is a lot more powerful an environment than the browser, so we need an alternate mapping, as specified [here](https://github.com/defunctzombie/node-browser-resolve#browser-field) An example: ```js { "browser": { "./lib/server": false }, "react-native": { "dgram": "react-native-udp", "fs": "react-native-level-fs" }, "chromeapp": { "dgram": "chrome-dgram", "fs": "level-filesystem" } } ``` on the other hand, if "react-native" is not present in package.json, you should fall back to "browser" other than the one (nesting) test added, the tests are unchanged, just done for both "react-native" and "browser" (I've implemented [react-native-udp](https://npmjs.org/package/react-native-udp) and [react-native-level-fs](https://npmjs.org/package/react-native-level-fs), but they obviously don't belong in the traditional "browser" field as they won't run anywhere except in React Native.) Closes facebook#2208 Reviewed By: svcscm Differential Revision: D2691236 Pulled By: vjeux fb-gh-sync-id: 34041ed50bda4ec07f31d1dc50dcdfa428af2512
Hey! Thanks a lot for this, I noticed a couple of things that are lacking in this implemenation.
if (replacements && typeof replacements === 'object') {
main = replacements['./' + main] ||
replacements['./' + main + '.js'] ||
replacements['./' + main + '.json'] ||
replacements['./' + main.replace(/(\.js|\.json)$/, '')] ||
replacements[main] ||
replacements[main + '.js'] ||
replacements[main + '.json'] ||
replacements[main.replace(/(\.js|\.json)$/, '')] ||
main;
function getReplacements(pkg) {
return pkg['react-native'] == null
? (pkg.browser == null ? pkg.browserify : pkg.browser)
: pkg['react-native'];
} |
@vespakoen if you'd like to submit a PR to fix this stuff, that would be great! |
Unfortunately I found my laptop on the floor this morning and my display is bricked, will get some monitor on monday and make a PR - written on my android |
What happened to this? |
The PR was accepted and merged into RN 0.17 I believe On Tue, Jul 19, 2016 at 23:45 Adrian Perez [email protected] wrote:
|
Did it? I must have looked in the wrong place, as in JavaScriptEngine Best Regards, Adrian Perez On July 20, 2016 at 7:47:35 PM, Chester Wood ([email protected])
|
It this working at the moment? My package.json looks like this: { and then in my code: var domain = require('domain'); and I get this: "Requiring unknown module "domain"...." message. any idea? |
facebookarchive/node-haste#77 for browser exclude support has been merged, but node-haste has not been republished to npm since. You can try overriding installing node-haste from github to react-native's node-haste dep locally |
Summary: React Native is a lot more powerful an environment than the browser, so we need an alternate mapping, as specified [here](https://github.com/defunctzombie/node-browser-resolve#browser-field) An example: ```js { "browser": { "./lib/server": false }, "react-native": { "dgram": "react-native-udp", "fs": "react-native-level-fs" }, "chromeapp": { "dgram": "chrome-dgram", "fs": "level-filesystem" } } ``` on the other hand, if "react-native" is not present in package.json, you should fall back to "browser" other than the one (nesting) test added, the tests are unchanged, just done for both "react-native" and "browser" (I've implemented [react-native-udp](https://npmjs.org/package/react-native-udp) and [react-native-level-fs](https://npmjs.org/package/react-native-level-fs), but they obviously don't belong in the traditional "browser" field as they won't run anywhere except in React Native.) Closes facebook/react-native#2208 Reviewed By: svcscm Differential Revision: D2691236 Pulled By: vjeux fb-gh-sync-id: 34041ed50bda4ec07f31d1dc50dcdfa428af2512
React Native used to have Map and Set polyfills that relied on tagging objects with a special __MAP_POLYFILL_INTERNAL_HASH__ property, which breaks down when the object to be tagged happens to be frozen. In 833072e (#3964) I introduced a workaround to make sure objects got tagged before becoming non-extensible, which robustly solved the problem for any Map and Set polyfills that rely on an object tagging strategy. Note that Apollo Client freezes objects only in development (using maybeDeepFreeze), so the problem fixPolyfills.ts was intended to solve was only ever a problem in development. I also submitted a PR to React Native that make their Map and Set polyfills store non-extensible objects in an array, rather than attempting to tag them: facebook/react-native#21492. Those changes were first released in React Native 0.59.0, so technically the fixPolyfills.ts logic should not be necessary for anyone using that version or later. Since then, React Native has stopped using any polyfills for Map and Set (yay!), so the need for the workaround has been even further reduced: facebook/react-native@93b9ac7 Those changes were first released in React Native 0.61.0, which is still the latest minor version (0.61.5 is latest). I'm not sure how many people are still using older versions of React Native, or what sort of LTS policies they have. Expo SDK 36 uses 0.61.4, for what it's worth: https://docs.expo.io/versions/latest/sdk/overview/ In any case, I think we can eliminate these polyfills from the default bundle, as long as we take some care to include them when bundling for React Native. This strategy uses a combination of techniques for selective bundling in React Native: https://facebook.github.io/react-native/docs/platform-specific-code#platform-specific-extensions facebook/react-native#2208
React Native used to have Map and Set polyfills that relied on tagging objects with a special __MAP_POLYFILL_INTERNAL_HASH__ property, which breaks down when the object to be tagged happens to be frozen. In 833072e (#3964) I introduced a workaround to make sure objects got tagged before becoming non-extensible, which robustly solved the problem for any Map and Set polyfills that rely on an object tagging strategy. Note that Apollo Client freezes objects only in development (using maybeDeepFreeze), so the problem fixPolyfills.ts was intended to solve was only ever a problem in development. I also submitted a PR to React Native that make their Map and Set polyfills store non-extensible objects in an array, rather than attempting to tag them: facebook/react-native#21492. Those changes were first released in React Native 0.59.0, so technically the fixPolyfills.ts logic should not be necessary for anyone using that version or later. Since then, React Native has stopped using any polyfills for Map and Set (yay!), so the need for the workaround has been even further reduced: facebook/react-native@93b9ac7 Those changes were first released in React Native 0.61.0, which is still the latest minor version (0.61.5 is latest). I'm not sure how many people are still using older versions of React Native, or what sort of LTS policies they have. Expo SDK 36 uses 0.61.4, for what it's worth: https://docs.expo.io/versions/latest/sdk/overview/ In any case, I think we can eliminate these polyfills from the default bundle, as long as we take some care to include them when bundling for React Native. This strategy uses a combination of techniques for selective bundling in React Native: https://facebook.github.io/react-native/docs/platform-specific-code#platform-specific-extensions facebook/react-native#2208
React Native is a lot more powerful an environment than the browser, so we need an alternate mapping, as specified here
An example:
on the other hand, if "react-native" is not present in package.json, you should fall back to "browser"
other than the one (nesting) test added, the tests are unchanged, just done for both "react-native" and "browser"
(I've implemented react-native-udp and react-native-level-fs, but they obviously don't belong in the traditional "browser" field as they won't run anywhere except in React Native.)