Skip to content

Conversation

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Oct 24, 2017

Added an automated test that would have caught the regression introduced with #11291 and fixed by #11350.

The new test verifies that this:

invariant(condition, 'This is not a real error message.');

Gets transformed to this:

!condition ? invariant(false, 'This is not a real error message.') : void 0;

Prior to #11291 it would not have been transformed (and so would have been slightly less efficient). After #11291 (with the regression) it would have failed with:

+ !condition ? __DEV__ ? invariant(false, 'This is not a real error message.') : 'This is not a real error message.' : void 0;

@gaearon
Copy link
Collaborator

gaearon commented Oct 24, 2017

Tangential but one downside of more efficient code is now we exercise the argument calculation code path less in tests. Which means that if it throws with some unusual inputs we might not know now. Because we'll only get invariant when we test specifically for it failing.

On the other hand as far as I understand this was already happening as soon as a code was recorded anyway.

Perhaps we should make an exception for tests and never wrap for them at all?

@gaearon
Copy link
Collaborator

gaearon commented Oct 24, 2017

While we're at it let's rename the test to match the transform new name?

@bvaughn
Copy link
Contributor Author

bvaughn commented Oct 25, 2017

Perhaps we should make an exception for tests and never wrap for them at all?

In general, I dig this suggestion, but I'm not sure how we'd make it work with tests like ReactDOMProduction "should throw with an error code in production". The transform runs on the code before the tests beforeEach hook sets the fake NODE_ENV.

I think we'd actually need to run against the production build (eg react-dom.production.min) which sounds nicer than mocking NODE_ENV but I'm not sure how to accomplish it. Maybe we could circle back here as a follow up after chatting?

While we're at it let's rename the test to match the transform new name?

Sure.

@bvaughn bvaughn force-pushed the error-code-invariant-test branch from 2e4905e to 1fa3a91 Compare October 25, 2017 01:52
@gaearon
Copy link
Collaborator

gaearon commented Oct 25, 2017

Sounds good.

@bvaughn bvaughn merged commit 72ba8db into facebook:master Oct 25, 2017
@bvaughn bvaughn deleted the error-code-invariant-test branch October 25, 2017 15:21
Ethan-Arrowood pushed a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017
…m regression (facebook#11356)

* Added an automated test that would have caught the recent error code transform bug

* Renamed dev-expression-with-codes test to replace-invariant-error-codes

* Formatting nit
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.

3 participants