Skip to content

Conversation

@gaearon
Copy link
Collaborator

@gaearon gaearon commented Mar 12, 2017

Since we now refer to it as fbjs/lib/invariant, this code path became dead.

I can’t actually reproduce any issue on master so I’m not sure I understand why this was necessary in the first place. @keyanzhang Maybe you can explain?

I have checked that the code path is hit again now after this change.

@sophiebits
Copy link
Collaborator

Did the jest test not fail? What does this fix?

@gaearon
Copy link
Collaborator Author

gaearon commented Mar 12, 2017

It did not fail because it was testing for a pattern we are not using anymore.
Jest test was verifying that

var invariant = require('invariant');

gets transformed into

var _prodInvariant = require('reactProdInvariant');
var invariant = require('invariant');

However we stopped using require('invariant') in #9078. So that part of the transform stopped running. The Jest test was verifying that a pattern that no longer exists in the codebase gets transformed.

I fixed both the transform and the test to reflect that it should be looking for require('fbjs/lib/invariant') instead.

As for what it fixes, I don’t know. I don’t understand why the original code was necessary. These are the lines that became dead code with #9078 (and that I’m trying to fix in this PR), but the prod invariant is inserted anyway later so maybe the branch I’m fixing is not necessary in the first place. This is the part I’m confused about.

Regardless, this PR just makes it run the same way it used to run before, and should be safe.

@keyz
Copy link
Contributor

keyz commented Mar 23, 2017

@gaearon you're right, the branch isn't really necessary.

Firstly when a require('invariant') call is found, the script inserts a var _prodInvariant = require('reactProdInvariant') statement. The getProdInvariantIdentifier() function saves the newly generated identifier name on the caller of the visiter function, which in effect ensures reactProdInvariant will only be inserted once. And then when a invariant(...) call itself is found, getProdInvariantIdentifier() finds the saved identifier name and replaces the call with it.

The reason why it still worked is because even though getProdInvariantIdentifier() fails to insert the prod invariant for require() calls, it inserts the require statement (once) while traversing invariant(...) calls anyway (a more accurate name of the helper function would be createOrGetProdInvariantIdentifier). Please feel free to remove the first if branch :)

@gaearon
Copy link
Collaborator Author

gaearon commented Mar 23, 2017

Want to send a PR? 😄
You're probably more confident making those changes. Renaming the function would be nice too.

@keyz
Copy link
Contributor

keyz commented Mar 23, 2017

Sure, I can just push to your branch if you don't mind

@gaearon
Copy link
Collaborator Author

gaearon commented Mar 28, 2017

Seems like it failed?

@keyz
Copy link
Contributor

keyz commented Mar 28, 2017

Yea sorry I forgot to update the tests. Now it should be fine!

@gaearon gaearon closed this Nov 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants