[Packager] Conditional require with RN Packager

[skevy]

The packager's dependency resolver doesn't respect conditional requires, and now, due to 793c356, these errors fail loudly.

As an example of this problem, in libs that use FBJS's promise polyfill, there is a deep dep on asap, which has this line: https://github.com/kriskowal/asap/blob/master/raw.js#L81

The packager fails hard on this (even though the require is conditional and not applicable here), with an error that looks like this:

[taion]

BTW, webpack doesn't have any special graceful handling for this. webpack resolves all requirements statically - the reason this doesn't cause issues in webpack is that node-libs-browser provides a domain module, which webpack uses.

IMO the correct way to handle this would be to add another field in package.json specific to React Native, maybe "react-native" by analogy with "browser". The reason I think this is semantically correct is because React Native is a distinct execution environment, so it'd be legitimate to override things specifically for React Native, instead of pretending it's a browser.

This would allow doing for fbjs:

"react-native": {
  "domain": false,
  "whatwg-fetch": false
}

for example.

[skevy]

cc @ide who I feel like has shared opinions in various places about the
react-native package.json field.
On Fri, Oct 16, 2015 at 2:24 PM Jimmy Jia [email protected] wrote:

BTW, webpack doesn't have any special graceful handling for this. webpack
resolves all requirements statically - the reason this doesn't cause issues
in webpack is that node-libs-browser provides a domain module, which
webpack uses.

IMO the correct way to handle this would be to add another field in
package.json specific to React Native, maybe "react-native" by analogy
with "browser". The reason I think this is semantically correct is
because React Native is a distinct execution environment, so it'd be
legitimate to override things specifically for React Native, instead of
pretending it's a browser.

This would allow doing for fbjs:

"react-native": {
"domain": false,
"whatwg-fetch": false
}

for example.

—
Reply to this email directly or view it on GitHub
#3463 (comment)
.

[ide]

The "react-native" field in package.json seems reasonable to me and there's precedent with the "browser" field. The important property is that the packager can statically determine what to put in the JS bundle(s).

What we don't want are conditional require() calls like this:

let Promise = (navigator.product === 'ReactNative') ? require('promise') : global.Promise;

The problem here is that require is not just a function. It's also a pragma that tells the packager what to include. So the require calls need to look the same across all environments, but each environment can provide a different implementation of the module, which is what the package.json entries achieve.

[taion]

👍

[skevy]

@vjeux you had some objections to the react-native field when we were chatting on Discord. Want to air them out here so we have it on record?

[vjeux]

@skevy would be nice if you could lay out the plan that we chatted about so that we can refer to it and have someone implement it :)

[skevy]

Yep.

So, one thing we could do is make the packager aware of the try { var xxx = require('xxx') } catch(e) { ... } pattern, and then mark the require as optional. To me, this seems complicated and prone to breakage, as it only handles so many use cases.

In order to handle dependencies like "domain", which is what's causing the particular problem I posted about, webpack uses this: https://github.com/webpack/node-libs-browser.

@vjeux mentioned he doesn't really want to maintain a list like this, which I get, but it would be the easiest way for a user to not have to worry about certain weird requires.

Honestly, I think the right way forward on this is both something like node-libs-browser for RN that we could build into the packager as well as the ability to override certain packages using package.json. That offers the MOST flexibility, and I think allows you to get around almost all situations. For all others - fork the package you're trying to get working :)

[ide]

Rather than bake the mappings into the packager, I'd much prefer the packager to support a way for people to provide their own mappings. This way we don't need to maintain a list of mappings, stave off PRs modifying the packager to change a mapping, and afford users the choice of mappings. I expect a few people will write blog posts or wikis on the mappings that work for them and get the word out that way.

I do want to keep the semantics of require/import as simple as we reasonably can. So many people already don't understand how require / JavaScript modules work - see the bug reports and questions about cyclic deps.

[skevy]

I want there to be a way for people to provide their own mappings. Huge proponent of that.

But in this case, FBJS (which will be used by RN at some point, presumably during the React 0.14 migration) uses promise which uses asap which references domain. Without the packager doing something about that, that's going to break unless someone puts

{
    "react-native": { "domain": false }
}

in their package.json. Of course, FBJS could somehow not rely on promise (seems unlikely though given that it's used on other FB projects), or we could encourage the asap author to somehow do that require in a different way...but I just feel like this issue could come up in other ways.

Idk...I hear what you're saying...I just worry about the ease of use of the pattern. For Relay for instance, people should be able to npm install relay and have it just work...while there's several things preventing that at the moment, this is definitely one of them.

[taion]

@ide

What do you mean "bake the mappings into the packager"? The "react-native" override would be per-package, I think.

In principle something like this could be annoying to manage for shared libraries, but I think in practice for managing e.g. "browser" v "node", it hasn't been that problematic.

[ide]

@taion my sentiment is that packager.js shouldn't hardcode a mapping from "os" to "os-browserify", for example. The mappings should live in the package.json files of react-native, fbjs, and any other npm package and be handled the same way.

@skevy thanks for the clarification - I think we're on the same page.

[skevy]

Alright so I guess we should work on a "react-native" package.json directive PR. Shouldn't be too hard. Guess I'll add it to my ever growing list.

But if anyone else wants to tackle it, go for it! 👍

[taion]

@ide

I think we're all on the same page, then. Per package.json-"react-native" directives are the cleanest way to go.

The only other question is whether it'd make sense to use node-libs-browser or an equivalent. It sounds like your inclination there is "no"?

[ide]

There's a bunch of unnecessary stuff in node-libs-browser. On several occasions I've repeatedly come to the conclusion that making Node libs work in a client environment is less useful than it sounds i.e. I don't think it's directionally correct for the RN project to endorse node-libs-browser. Pulling in polyfills as needed is a better approach for professional-quality projects.

[skevy]

Oh - someone already implemented this: #2208

What are the chances we could move that onto somebody's radar for review?

[vjeux]

cc @martinbigio @amasad

[skevy]

also cc @mvayngrib, the author of #2208

[mvayngrib]

@ide I disagree that enabling a Node-like environment in RN is not useful. The Node community is huge, prolific, and this is one way of engaging them more in RN. The browserify project did a similar thing to great effect. That said, node-libs-browser would undoubtedly need to be tweaked to something that works in RN. I currently recreate a Node env in RN with a few shims for core modules (react-native-level-fs, react-native-udp, asyncstorage-down), plus some hacks around packager, and it immediately lets me use existing node libs from the level ecosystem, streaming, networking, bittorrent, blockchain, etc, which don't work otherwise. People could be creating Popcorn Time or the like with RN, and this is holding them back. So personally, I'd love to see Node in RN!

[skevy]

Things referenced in this issue are closed and merged. Closing.