Better compatibility with browserify modules

[vespakoen]

This pull request does a couple of things

1. Checks for the browserify field (some old packages still depend on it)
   https://github.com/facebook/node-haste/pull/46/files#diff-9103f1ac13c3018f7108b3e6bbf20df4R125

2. Handles false in replacements, shimming it with _empty.js as browserify does
   https://github.com/facebook/node-haste/pull/46/files#diff-9103f1ac13c3018f7108b3e6bbf20df4R99

3. Add function to get a replacement for a file that is also used in getMain (with more path checks)
   https://github.com/facebook/node-haste/pull/46/files#diff-9103f1ac13c3018f7108b3e6bbf20df4R51

It checks for name, './' + name and './' + path.relative(this.root, name);
And we should probably add path.relative(this.root, name); to that as well?

4. Cherry on the cake, shims browserify builtins
   https://github.com/facebook/node-haste/pull/46/files#diff-9103f1ac13c3018f7108b3e6bbf20df4R87

I tested these changes by requiring some modules and inspecting the generated bundle while on the latest tag of react-native (0.20) and later copied them over to my node-haste fork.

Simply said, It very likely contains bugs and is probably incompatible with the current master branch.

Tests will follow soon, but here it is anyways!

There is room for improvement and help is welcome.

Questions

1. If the path starts with node_modules, it doesn't work anymore, see my "hotfix" here
   https://github.com/facebook/node-haste/pull/46/files#diff-9103f1ac13c3018f7108b3e6bbf20df4R91

2. Not every module that is (removed /) shimmed with false is replaced.
   It seems to work fine when the replacement is "at the root" but not when "nested deeper", see the examples of what I mean by that:

root:

"react-native": {
  "./something.js": false
}

nested deeper:

"react-native": {
  "./lib/something.js": false
}

https://github.com/facebook/node-haste/pull/46/files#diff-9103f1ac13c3018f7108b3e6bbf20df4R102

Todos

• Adding tests
• Figuring out how to move forward with browserify builtins / global replacements
• Figuring out what causes question 1
• Figuring out what causes question 2

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[vespakoen]

Pulling in the experts @mvayngrib, @amasad, @martinbigio, @defunctzombie

[amasad]

I don't think we're aiming for 100% compatibility with browserify. For example, builtins doesn't make sense for react-native and jest handles this on it's own.

I think we're aiming for compatibility with the spec in a way that is meaningful for the platforms we support. Is any of this related to the spec?

[vespakoen]

Alright,

My opinion is the more compatibility with npm packages the better.
I don't understand your comment about jest, it's a test framework right? I fail to see the link, can you please elaborate on that?

Referring to my points from above:

1. Checking the browserify field (which is vintage, but is still present in a lot of packages) will not break anything, it isn't in the spec so I can understand leaving it out and opening issues over at the packages that still use it but since it isn't harmful (compatibility++), I vote for leaving it in there.

2. This is in the spec (and not handled in the current implementation) https://github.com/defunctzombie/package-browser-field-spec#ignore-a-module

3. This is not in the spec, but in node-browser-resolved, which is used by browserify (and from the same guy that made the spec!)

Basically checking with and without ./

https://github.com/defunctzombie/node-browser-resolve/blob/master/index.js#L171-L172

4. Not in the spec, but a welcome addition in my opinion.

A lot of modules (at least the ones I am familiar with and use a lot) depend on browserify's builtins.
To be able to use packages that depend on browserify's builtins without this is quite a mess (we have to resort to something like this: https://github.com/mvayngrib/rn-nodeify to "hack" package.json's) or make a ton of forks just to get things working.

I would even go further and suggest to add the global and process shims into a default build as well (https://github.com/mvayngrib/rn-nodeify/blob/master/shim.js)

I wonder what the arguments are for not having this kind of compatibility?

[amasad]

I don't understand your comment about jest, it's a test framework right? I fail to see the link, can you please elaborate on that?

3 main clients for this module now: Facebook internal, React Native open source, and Jest open source. We don't intend to maintain this as a publicly consumable library, it's just common infrastructure for our open source projects -- therefore any decisions that we make here should be purely driven by any one of those clients.

1. Checking the browserify field (which is vintage, but is still present in a lot of packages) will not break anything, it isn't in the spec so I can understand leaving it out and opening issues over at the packages that still use it but since it isn't harmful (compatibility++), I vote for leaving it in there.

Fine with this.

2. This is in the spec (and not handled in the current implementation) https://github.com/defunctzombie/package-browser-field-spec#ignore-a-module

Fine with this too. But can we implement our own empty.js instead of depending on browserify?

3. This is not in the spec, but in node-browser-resolved, which is used by browserify (and from the same guy that made the spec!)

OK, but please add tests for these cases.

4. Not in the spec, but a welcome addition in my opinion.
   I wonder what the arguments are for not having this kind of compatibility?

As mentioned above, I think this should be driven by one of the consumers. If react-native wants this then we can add it. However, last I remember I was part of the conversation the consensus was that we don't want to add builtins wholesale. Anyhow, you should bring it up with the React Native team. Feel free to open an issue on the repo and cc me :)

For now, I'm fine with the #1 and #2 and #3 provided you add some tests -- as you can see the library is heavily tested as the resolution algorithm can get tricky to change and test.

[vespakoen]

Ah, makes sense now, I didn't know there were other consumers of this package.

Would you support supporting the browserify builtins behind some flag?
Or make them configurable in the project's package.json?

"global-react-native": {
  "stuff-that-needs-to-be-gone": false,
  "fs": "react-native-fs"
}

I think this is a very important functionality since it's so hard to emulate in another way.

Working on the tests improvements and syntax as we speak ;)

[amasad]

Open to it but only if React Native is. I don't have enough context to judge what the trade-offs are. I encourage you to open the issue on the React Native repo (or find an existing one).

[vespakoen]

A little update,

I am having trouble writing a (passing) test for the _empty.js shim, I have tried moving it into a new package here but even with that, I cannot mock the package and have the test "swallow" it haha, help would be greatly appreciated!

Not getting any responses over at the react-native project too

facebook/react-native#6253

[davidaurelio]

Hi @vespakoen, sorry for letting you wait.

@cpojer and me are currently very limited on time, so please bear with us.

[vespakoen]

No worries, I have been busy last time and on a 8 hour bus-ride wasn't able to get jest to swallow the package shim

• I have tried using the decorator to "provide" the package
• I tried code that looks up the path of my shim package (via require.resolve), turn the path into one big nested object and add that to the mockFs

All without success unfortunately.

Some progress I made

• I was able to load the project's package.json and load the "global replacements" from there and not from the browserify built-ins file anymore, but this is not optimized for speed yet so I haven't pushed it.

Will dabble with it again for a bit now and report back later.

[vespakoen]

Good news,

I have some working code again (based on the latest node-haste this time).

With it (besides adding window.process = require('process')) PouchDB seems to work!

I made an example project that shows it in action over here: https://github.com/vespakoen/RNNPMCompat

The latest code is on a new branch:
https://github.com/vespakoen/node-haste/tree/global-react-native

In summary, the global-react-native branch brings:

• Browser exclude (as seen in #62) - My pull requests shims the excluded files with an empty file, preventing the red screen from popping up as react-native tries to require a non existing file. Is this the right way to go about this case? Thanks for showing me a different approach @mvayngrib!
• "Global require replace" aka alias.resolve. - Allows package wide replacements
• Merges react-native on top of browser - Copied from pull request https://github.com/facebook/node-haste/pull/61/files, I took a bit of a different approach, let's see what works best. Thanks again @mvayngrib!

I couldn't get the tests to run at all now, (I also tried running "jest src/tests/**" from the root of the node-haste project, without success)

[vespakoen]

Can someone help writing a test case for this? I guess I hit a gnarly edge of jest's possibilities 😉

[davidaurelio]

Jest has switched from node-haste to jest-haste-map

[cpojer]

@davidaurelio I think this is about writing a test for node-haste (using Jest).