[Packager] Fix @providesModule not being ignored properly #3625

skevy · 2015-10-23T04:41:11Z

There's quite a bit of code scattered around the packager regarding ignoring the @providesModule Haste pragma in any file that isn't in react-native, react-tools or parse. There is even a (passing) test case.

However, there's an edge case.

Take, for example, fbjs. It has a module inside of it called ErrorUtils. react-relay requires this file normally, in Common.JS style, by doing require('fbjs/libs/ErrorUtils'). But when react-native attempts to require ErrorUtils using the HasteModule format (in it's JavaScript initialization), it resolves the fbjs ErrorUtils module, instead of RN's ErrorUtils.

This happens, it turns out, because when a module is read (in Module._read), it's not caring about whether or not it should pay attention to @providesModule, and is just assigning the @providesModule value as the id of the module no matter what. Then when Module.getName is called, it will always use that data.id that was set, thus creating the wrong dependency tree.

This PR fixes that. :)

facebook-github-bot · 2015-10-23T04:41:27Z

By analyzing the blame information on this pull request, we identified @martinbigio, @amasad and @vjeux to be potential reviewers.

ide · 2015-10-23T07:34:05Z

packager/react-packager/src/DependencyResolver/__tests__/Module-test.js

@@ -22,6 +22,7 @@ jest
 var Fastfs = require('../fastfs');
 var Module = require('../Module');
 var ModuleCache = require('../ModuleCache');
+var Helpers = require('../DependencyGraph/Helpers');


We might want to rename this module to be more descriptive (not sure what though. DependencyGraphHelpers?) since it's being used outside of DependencyGraph now.

Totally agree.

facebook-github-bot · 2015-10-23T14:36:23Z

@skevy updated the pull request.

skevy · 2015-11-09T18:20:14Z

@martinbigio should I update and rebase this PR, or are you guys fixing this issue in a different way?

vjeux · 2015-11-09T18:48:58Z

@skevy fyi Martin is in PTO for three weeks starting today :x Let me take it

skevy · 2015-11-09T18:51:18Z

Yay time off! I wish I could get some of that. :(

skevy · 2015-11-09T18:51:28Z

Thanks for taking over!

vjeux · 2015-11-09T18:55:37Z

@facebook-github-bot import

facebook-github-bot · 2015-11-09T18:58:27Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/417058798495156/int_phab to review.

vjeux · 2015-11-09T19:05:33Z

packager/react-packager/src/DependencyResolver/Module.js

@@ -85,7 +86,7 @@ class Module {
      this._reading = this._fastfs.readFile(this.path).then(content => {
        const data = {};
        const moduleDocBlock = docblock.parseAsObject(content);
-        if (moduleDocBlock.providesModule || moduleDocBlock.provides) {
+        if (!this._helpers.isNodeModulesDir(this.path) && (moduleDocBlock.providesModule || moduleDocBlock.provides)) {


nit: 80 columns.

if (!this._helpers.isNodeModulesDir(this.path) && (moduleDocBlock.providesModule || moduleDocBlock.provides)) {

Personally I prefer:

if (!this._helpers.isNodeModulesDir(this.path) && (moduleDocBlock.providesModule || moduleDocBlock.provides) ) {

but I don't wanna bikeshed :P

facebook-github-bot · 2015-11-09T23:37:15Z

@skevy updated the pull request.

vjeux · 2015-11-10T00:41:23Z

@facebook-github-bot import

facebook-github-bot · 2015-11-10T00:43:27Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/417058798495156/int_phab to review.

vjeux · 2015-11-10T01:03:57Z

   Error  (FBESLINT0) ESLint reported an error.
    "fileWatcher" is not defined. (no-undef)

            2325 
            2326       var dgraph = new DependencyGraph({
            2327         roots: [root],
    >>>     2328         fileWatcher: fileWatcher,
            2329         assetExts: ['png', 'jpg'],
            2330         cache: cache,
            2331       });

   Error  (FBESLINT0) ESLint reported an error.
    "cache" is not defined. (no-undef)

            2327         roots: [root],
            2328         fileWatcher: fileWatcher,
            2329         assetExts: ['png', 'jpg'],
    >>>     2330         cache: cache,
            2331       });
            2332       return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').then(function(deps) {
            2333         expect(deps)

The tests are not passing because it uses variables that are never defined :x

vjeux · 2015-11-10T01:06:18Z

Could you rebase? :) I have no idea what i'm doing here :x

skevy · 2015-11-10T01:08:49Z

Sorry @vjeux, I didn't mean to push. I'm working on this now.

skevy · 2015-11-10T01:48:20Z

This issue with rebasing definitely has something to do with the work @cpojer is doing on the packager I think. I need to take a little bit of a look at what he's up to, and complete the rebase tomorrow when I'm fresh. Thanks for being johnny on the spot with this @vjeux...hopefully we can get it all fixed up and ready to ship tomorrow! :)

cpojer · 2015-11-10T02:04:03Z

I'm about to pull out the resolver into a separate repo sometime this week. It would be great if you could rebase this asap and land it, so I don't run into issues with syncing this. Sorry for the trouble!

cpojer · 2015-11-10T02:04:47Z

packager/react-packager/src/DependencyResolver/Module.js

@@ -15,7 +15,7 @@ const replacePatterns = require('./replacePatterns');

 class Module {

-  constructor(file, fastfs, moduleCache, cache) {
+  constructor(file, fastfs, moduleCache, cache, helpers) {


we should probably change this into taking a single object, and use destructuring in here.

skevy · 2015-12-02T15:16:18Z

@martinbigio yah I'm reopening this now, because we actually determined that the test failures were a Facebook internal infra problem (related to a babel-transform that isn't open sourced), not a problem with this PR.

Wait till @vjeux tells you about this one :)

martinbigio · 2015-12-03T22:36:24Z

Oh yeah, he told me how you guys fixed it. XP for the win :)

martinbigio · 2015-12-03T22:37:06Z

@cpojer do any of the modified file should be modified on node-haste as well?

martinbigio · 2015-12-03T22:39:35Z

packager/react-packager/src/DependencyResolver/Module.js

        const moduleDocBlock = docblock.parseAsObject(content);
-        if (moduleDocBlock.providesModule || moduleDocBlock.provides) {
+        if (!this._depGraphHelpers.isNodeModulesDir(this.path) &&


Maybe we should add a comment explaining why checking for this is necessary. It's not immediately obvious to me

martinbigio · 2015-12-03T22:42:53Z

Looks great to me. @cpojer what do you want to do with the changes that affect node-haste? How far away are we from getting rid of the duplicated code? Maybe we should think about a strategy to avoid missing changes.

facebook-github-bot · 2015-12-04T00:33:17Z

@skevy updated the pull request.

skevy · 2015-12-04T00:34:20Z

packager/react-packager/src/DependencyResolver/Module.js

+        // This handles the case where a project may have a dep that has @providesModule
+        // docblock comments, but doesn't want it to conflict with whitelisted @providesModule
+        // modules, such as react-haste, fbjs-haste, or react-native or with non-dependency,
+        // project-specific code that is using @providesModule.


@martinbigio check out this explanation and let me know if that's good.

facebook-github-bot · 2015-12-04T00:41:13Z

@skevy updated the pull request.

facebook-github-bot · 2015-12-04T00:43:16Z

@skevy updated the pull request.

martinbigio · 2015-12-04T16:19:46Z

I'll chat with @cpojer to see how we can pull changes into the new resolver repo

facebook-github-bot · 2015-12-24T14:44:18Z

@skevy updated the pull request.

…cyGraph This resolves the packager not properly ignoring @providesModule when naming modules.

…ad() Adds a comment that explains, in detail, why we should check whether the dependency is in the node_modules directory (or a whitelisted node_module) before getting it's name from @providesModule

…_modules' test case. Rather than adding an additional test case for selectively whitelisted @providesModule, add to existing test case more robust tests that handle the previously unaccounted-for use case.

…le classes. Renaming `_read` method in Module.js and Packager.js to `read`, as to not be a private method. This is done in order to allow for proper class inheritance, even when using Facebook's internal "babel-plugin-class-private-props" transform, which actually mangles method names starting with '_', preventing child classes from overriding the method.

facebook-github-bot · 2015-12-24T15:11:13Z

@skevy updated the pull request.

skevy · 2015-12-24T15:13:51Z

packager/react-packager/src/DependencyResolver/Module.js

  }

  invalidate() {
    this._cache.invalidate(this.path);
  }

-  _read() {
+  read() {


@vjeux just pointing out that I renamed this method as we discussed, as to allow for proper class inheritance and to not break due to babel-plugin-class-private-props.

cc @martinbigio

vjeux · 2015-12-24T15:22:12Z

@facebook-github-bot shipit

facebook-github-bot · 2015-12-24T15:22:21Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/417058798495156/int_phab to review.

skevy · 2015-12-24T16:52:47Z

Yay this finally merged! Thanks @vjeux

Summary: There's quite a bit of code scattered around the packager regarding ignoring the `providesModule` Haste pragma in any file that isn't in `react-native`, `react-tools` or `parse`. There is even a (passing) test case. However, there's an edge case. Take, for example, `fbjs`. It has a module inside of it called `ErrorUtils`. `react-relay` requires this file normally, in Common.JS style, by doing `require('fbjs/libs/ErrorUtils')`. But when `react-native` attempts to require `ErrorUtils` using the HasteModule format (in it's JavaScript initialization), it resolves the `fbjs` `ErrorUtils` module, instead of RN's `ErrorUtils`. This happens, it turns out, because when a module is read (in `Module._read`), it's not caring about whether or not it should pay attention to `providesModule`, and is just assigning the `providesModule` value as the id of the module no matter what. Then when `Module.getName` is called, it will always use that `data.id` that was set, thus creating the wrong dependency tree. This Closes facebook/react-native#3625 Reviewed By: svcscm Differential Revision: D2632317 Pulled By: vjeux fb-gh-sync-id: efd8066eaf6f18fcf79698beab36cab90bf5cd6d

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Oct 23, 2015

skevy mentioned this pull request Oct 23, 2015

[Packager] Handling duplicate Haste modules by scoping them to npm packages #3618

Closed

ide assigned martinbigio Oct 23, 2015

ide reviewed Oct 23, 2015
View reviewed changes

ide added the Packager label Oct 23, 2015

brysgo mentioned this pull request Oct 26, 2015

Make Relay work with React Native out of the box facebook/relay#26

Closed

5 tasks

vjeux assigned vjeux and unassigned martinbigio Nov 9, 2015

vjeux reviewed Nov 9, 2015
View reviewed changes

ide added GH Review: needs-revision and removed GH Review: review-needed labels Nov 10, 2015

cpojer reviewed Nov 10, 2015
View reviewed changes

skevy reopened this Dec 2, 2015

martinbigio reviewed Dec 3, 2015
View reviewed changes

skevy reviewed Dec 4, 2015
View reviewed changes

skevy force-pushed the providesModule-packager-fix branch from 74821ac to 7b69278 Compare December 4, 2015 00:42

skevy force-pushed the providesModule-packager-fix branch from 7b69278 to eab854c Compare December 24, 2015 14:43

skevy added the Size/L label Dec 24, 2015

skevy added 4 commits December 24, 2015 10:08

[Packager] Fix @providesModule not being ignored properly by Dependen…

2853a9d

…cyGraph This resolves the packager not properly ignoring @providesModule when naming modules.

[Packager] Comment @providesModule ignore functionality in Module._re…

52bfcdd

…ad() Adds a comment that explains, in detail, why we should check whether the dependency is in the node_modules directory (or a whitelisted node_module) before getting it's name from @providesModule

[Packager] Refactor 'should selectively ignore providesModule in node…

b5edcee

…_modules' test case. Rather than adding an additional test case for selectively whitelisted @providesModule, add to existing test case more robust tests that handle the previously unaccounted-for use case.

skevy force-pushed the providesModule-packager-fix branch from eab854c to 1ddc0ab Compare December 24, 2015 15:10

skevy reviewed Dec 24, 2015
View reviewed changes

skevy added GH Review: review-needed and removed GH Review: needs-revision labels Dec 24, 2015

skevy closed this in 6cec263 Dec 24, 2015

jwhitley mentioned this pull request Mar 5, 2016

npm test fails with naming collision benoitvallon/react-native-nw-react-calculator#23

Closed

[Packager] Fix @providesModule not being ignored properly #3625

[Packager] Fix @providesModule not being ignored properly #3625

Conversation

skevy commented Oct 23, 2015

facebook-github-bot commented Oct 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 23, 2015

skevy commented Nov 9, 2015

vjeux commented Nov 9, 2015

skevy commented Nov 9, 2015

skevy commented Nov 9, 2015

vjeux commented Nov 9, 2015

facebook-github-bot commented Nov 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 9, 2015

vjeux commented Nov 10, 2015

facebook-github-bot commented Nov 10, 2015

vjeux commented Nov 10, 2015

vjeux commented Nov 10, 2015

skevy commented Nov 10, 2015

skevy commented Nov 10, 2015

cpojer commented Nov 10, 2015

Choose a reason for hiding this comment

skevy commented Dec 2, 2015

martinbigio commented Dec 3, 2015

martinbigio commented Dec 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinbigio commented Dec 3, 2015

facebook-github-bot commented Dec 4, 2015

Choose a reason for hiding this comment

facebook-github-bot commented Dec 4, 2015

facebook-github-bot commented Dec 4, 2015

martinbigio commented Dec 4, 2015

facebook-github-bot commented Dec 24, 2015

facebook-github-bot commented Dec 24, 2015

Choose a reason for hiding this comment

vjeux commented Dec 24, 2015

facebook-github-bot commented Dec 24, 2015

skevy commented Dec 24, 2015