-
Notifications
You must be signed in to change notification settings - Fork 70
Better compatibility with browserify modules #46
base: master
Are you sure you want to change the base?
Conversation
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 [email protected]. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Pulling in the experts @mvayngrib, @amasad, @martinbigio, @defunctzombie |
I don't think we're aiming for 100% compatibility with browserify. For example, builtins doesn't make sense for 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? |
Alright, My opinion is the more compatibility with npm packages the better. Referring to my points from above:
Basically checking with and without https://github.com/defunctzombie/node-browser-resolve/blob/master/index.js#L171-L172
A lot of modules (at least the ones I am familiar with and use a lot) depend on browserify's builtins. I would even go further and suggest to add the I wonder what the arguments are for not having this kind of compatibility? |
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.
Fine with this.
Fine with this too. But can we implement our own
OK, but please add tests for these cases.
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. |
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? "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 ;) |
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). |
A little update, I am having trouble writing a (passing) test for the Not getting any responses over at the |
Hi @vespakoen, sorry for letting you wait. @cpojer and me are currently very limited on time, so please bear with us. |
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
All without success unfortunately. Some progress I made
Will dabble with it again for a bit now and report back later. |
Good news, I have some working code again (based on the latest node-haste this time). With it (besides adding 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: In summary, the
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) |
Can someone help writing a test case for this? I guess I hit a gnarly edge of jest's possibilities 😉 |
Jest has switched from node-haste to jest-haste-map |
@davidaurelio I think this is about writing a test for node-haste (using Jest). |
This pull request does a couple of things
Checks for the browserify field (some old packages still depend on it)
https://github.com/facebook/node-haste/pull/46/files#diff-9103f1ac13c3018f7108b3e6bbf20df4R125
Handles
false
in replacements, shimming it with_empty.js
as browserify doeshttps://github.com/facebook/node-haste/pull/46/files#diff-9103f1ac13c3018f7108b3e6bbf20df4R99
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?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
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
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:
nested deeper:
https://github.com/facebook/node-haste/pull/46/files#diff-9103f1ac13c3018f7108b3e6bbf20df4R102
Todos