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

Consider browserify field #8

Merged
merged 1 commit into from
Feb 23, 2016

Conversation

vespakoen
Copy link
Contributor

@vespakoen
Copy link
Contributor Author

Note that I remove the "react-native" and "browserify" field in the end, this makes sure the hacked "browser" is used by the react-native packager.

Also see my comment on this thread: facebook/react-native#2208 for more compatibility fixes.

@mvayngrib
Copy link
Member

@vespakoen lgtm, maybe just squash the commits. I was thinking actually that we'd start using the "react-native" field, now that it's supported, but leave the "browser" field as a fallback. Do you mind adjusting it to do that?

@vespakoen
Copy link
Contributor Author

Will do! hang in there...

@vespakoen
Copy link
Contributor Author

Updated and squashed, looks good?

@mvayngrib
Copy link
Member

yep, but let's leave both "react-native" and "browser", so it works for older react-native versions

@vespakoen
Copy link
Contributor Author

Sounds like a good idea, just a sec

@vespakoen
Copy link
Contributor Author

Like that?

@@ -237,7 +237,8 @@ function fixPackageJSON (modules, file, overwrite) {
})

if (!deepEqual(orgBrowser, depBrowser)) {
pkgJson.browser = depBrowser
Copy link
Member

Choose a reason for hiding this comment

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

almost, just add back pkgJson.browser = depBrowser, to have both browser and react-native

Edit:
i mean pkgJson.browser = pkgJson['react-native'] = depBrowser
and could you check that it actually works? Sometimes these improvements piss react-packager off

@vespakoen
Copy link
Contributor Author

Ahh good catch!

Will update, test and report back

@vespakoen
Copy link
Contributor Author

Confirmed & works!

...
  "browser": {
    "crypto": "react-native-crypto",
    "http": "react-native-http",
    "querystring": "querystring-es3",
    "_stream_transform": "readable-stream/transform",
    "_stream_readable": "readable-stream/readable",
    "_stream_writable": "readable-stream/writable",
    "_stream_duplex": "readable-stream/duplex",
    "_stream_passthrough": "readable-stream/passthrough",
    "stream": "stream-browserify"
  },
  "react-native": {
    "crypto": "react-native-crypto",
    "http": "react-native-http",
    "querystring": "querystring-es3",
    "_stream_transform": "readable-stream/transform",
    "_stream_readable": "readable-stream/readable",
    "_stream_writable": "readable-stream/writable",
    "_stream_duplex": "readable-stream/duplex",
    "_stream_passthrough": "readable-stream/passthrough",
    "stream": "stream-browserify"
  },
...

mvayngrib added a commit that referenced this pull request Feb 23, 2016
@mvayngrib mvayngrib merged commit a7e2d0e into tradle:master Feb 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants