Using some 3rd party libraries with React Native is not possible due … #13980
Conversation
…to their reliance on some CommonJS/node module behavior not implemented by the React Native require polyfill from the packager. This PR adds some of these behaviors: Circular dependencies when exporting by assignment to the module.exports object. Should fix facebook#3298 for good. Exporting by assignment to the this object. Fixes facebook#2862 The module.id represents a unique identifier that can be used in require() calls. There were no tests for the require polyfill so I added one.
|
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! If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions. |
|
Just commenting because we needed to do a manual update to pick up the CLA check. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
cc @cpojer |
| set exports(value) { | ||
| module.exports = value; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Why did you decide to use getters/setters? I would prefer plain properties.
| if (__DEV__ && module.hot) { | ||
| moduleObject.hot = module.hot; | ||
| } | ||
|
|
||
| // keep args in sync with with defineModuleCode in | ||
| // packager/src//Resolver/index.js | ||
| // and packager/src//ModuleGraph/worker.js | ||
| factory(global, require, moduleObject, exports, dependencyMap); | ||
| factory.call(module.exports, global, require, moduleObject, module.exports, dependencyMap); |
There was a problem hiding this comment.
I am aware that node.js does this. Are you aware of any modules that actually depend on this? We will have to run some internal perf tests on this to gauge real-world impact.
| const moduleObject = { | ||
| get id() { | ||
| return moduleId; | ||
| }, |
There was a problem hiding this comment.
This id is meaningless for programmatic use. What profits from having it?
|
Ah, I get it. You need getter/setter to support cyclic dependencies. We won't support that. It's too much overhead, memory and perf-wise. I am on mobile, but iirc node uses plain properties, too. |
|
@davidaurelio I am confused between your comments and last statement. Are you suggesting that you don't want to support this given that it may add overhead? We have been running this for our application that needed cyclical dependency resolution. |
|
I can run perf tests on this, it will then also include the overhead for Many cyclic dependencies can be solved by factoring out the common parts into a third module. I will give you perf data on Monday. |
|
Thanks a lot, looking forward to the result. |
facebook-github-bot
added
the
Import Started
|
@davidaurelio has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Hi there! The packager no longer lives in this repo. It now lives in https://github.com/facebook/metro-bundler. The reasoning for the split is explained here: #13976. If you're still interested in pursuing this PR, please open the PR on that repo! :) Thanks! |
… was part of that and it was suggested to merge with this project. Link to original request facebook/react-native#13980 …to their reliance on some CommonJS/node module behavior not implemented by the React Native require polyfill from the packager. This PR adds some of these behaviors: Circular dependencies when exporting by assignment to the module.exports object. Should fix #3298 for good. Exporting by assignment to this object. Fixes #2862 The module.id represents a unique identifier that can be used in require() calls. There were no tests for the require polyfill so I added one. Motivation (required) What existing problem does the pull request solve? Circular dependencies when exporting by assignment to the module.exports object. Should fix #3298 for good. Exporting by assignment to this object. Fixes #2862 Test Plan (required) There is no change to UI or API. However, tests have been added for require.js polyfill. Next Steps Individual and company CLA have been signed already.
…to their reliance on some CommonJS/node module behavior not implemented by the React Native require polyfill from the packager.
This PR adds some of these behaviors:
Circular dependencies when exporting by assignment to the module.exports object. Should fix #3298 for good.
Exporting by assignment to this object. Fixes #2862
The module.id represents a unique identifier that can be used in require() calls.
There were no tests for the require polyfill so I added one.
Motivation (required)
What existing problem does the pull request solve?
Circular dependencies when exporting by assignment to the module.exports object. Should fix #3298 for good.
Exporting by assignment to this object. Fixes #2862
Test Plan (required)
There is no change to UI or API. However, tests have been added for require.js polyfill.
Next Steps
Individual and company CLA have been signed already.