Bugfix/require module regexp

[daviskoh]

Resolves #316. Also updated the spec for the Haste Dependency Resolver. Not sure if these changes are the ones desired so feedback would be welcome!

[amasad]

Awesome, thanks!

[tadeuzagallo]

I think the regex should be

/\brequire\s*?\(\s*?([\'"])([^"\']+)\1\s*?\)/g

and then

code.replace(REL_REQUIRE_STMT, function(codeMatch, _, depName) {

Even before this diff it already accepted require('module")
And \s* has a very poor performance when you expect \s to be very small, \s*? should perform much better.

[daviskoh]

👍 updated

[amasad]

If you updated this, let's update the one in DependencyGraph/index.js as well. Ideally let's export it from DependencyGraph as a a constant to be used here

[daviskoh]

Hmm. Maybe I am missing something, but I am facing some issues when I try to export the requireRe regex from DependencyGraph and into DependencyResolver.

I changed the requireRe in the DependencyGraph to

var requireRe = /\brequire\s*?\(\s*?([\'"])([^"\']+)\1\s*?\)/g;

Then changed the exports

exports.requireRe = requireRe;
exports.graph = DependecyGraph;

Then in the DependencyResolver/index.js

var DependencyGraph = require('./DependencyGraph').graph;
var requireRe = require('./DependencyGraph').requireRe;
console.log(requireRe);

Jest output

The requireRe regex does not seem to be importing properly as you can see in the Jest output. I will keep digging into this. Any help would be appreciated as well!

[daviskoh]

I changed the requireRe in the DependencyGraph to match REL_REQUIRE_STMT.

As noted above, I was having issues when I tried to import regex into the DependencyResolver. As @amasad said, ideally we would export the const, however in the meantime this should provide a fix for the whitespace issues.

I can submit another PR later exporting the requireRe when I can look at this code more objectively than I can now. What do you think?

[amasad]

That's strange, maybe the problem is with jest console.log. Can you try console.log(requireRe.toString())?

[daviskoh]

This is the output from DependencyResolver/index.js:

It does not seem limited to importing from DependencyGraph either. I tried creating a dummy file that exports only that regex and I get the same issue. I also tried exporting some simple regex:

/\s+/g

but it seems to get imported as

/(?:)/

as well. T.T

[daviskoh]

It does not seem to be an issue w/ the console.log of jest because when I

console.log(requireRe);

right after it is declared within DependencyGraph, the output is:

[daviskoh]

The issue was related to how Jest seems to auto-mock its dependencies (though I am not too familiar w/ Jest). When I tried to export the regex, jest would try to mock it and resulting in that strange looking regex that broke unit tests (even when I tried mocking it as a function that returns the regex). However, I tested the regex exporting manually and it was working just fine.

I decided that the regex value should not be mocked in the unit tests. However, keeping this value inside the DependencyGraph/index.js was problematic as we still want to mock the rest of the DependencyGraph when testing the HasteDependencyResolver. Thus, I decided to move the regex value into a separate file that gets imported by both DependencyGraph/index.js & DependencyResolver/index.js allowing me to prevent the value from being mocked using jest.dontMock.

I named it regex.js as I thought there may be other regex where sharing across files is desired. The shared regex is now successfully exported and the unit tests are now passing.

@amasad thoughts?

[pilwon]

Related to #386

[amasad]

That looks good, I'll pull it.

[daviskoh]

👍

[pilwon]

Do we really need to force convert quotation style (' or ") to single quotations when we relativize code? Wouldn't it be better we preserve whatever style developer chose?

[amasad]

@pilwon why do you care about generated code?

[pilwon]

@amasad I agree it doesn't matter for code that is never meant to be read by humans.

I imagined more potential complex case similar to and developed from how I implemented #386. The original code passes through multiple replace steps until it reaches the final relativized code. The complexity in this replace pipeline could potentially grow (though it's highly unlikely beyond supporting import syntaxes for now) and it may be a better design choice to replace only whatever's absolutely necessary in each replace step. Either choice definitely works fine but forced conversion of quotation style didn't seem like a necessary one for me. (I'm probably overthinking this...)

[amasad]

OK, this already landed internally though. @pilwon you can fix it in your "import" syntax pull requests because I haven't looked at it yet.

[daviskoh]

Hmm...that explains the merge conflicts lol. I guess there must be a problem w/ Legal when taking PR's from random internet guys ☔ hehe

[pilwon]

@daviskoh Related info here

@amasad I'm not sure how to rebase your internal code onto PR #386. The branch must have been diverged since this PR and my PR touched the same files. When you bring the changes to your internal code, is it usually a manual process? Should I wait until this PR makes its way back to react-native? (Updated PR #386)

[sahrens]

Merged internally and synced.