Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Loosen assertion so it matches any production error #8907

Merged
merged 2 commits into from
Feb 2, 2017

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Jan 31, 2017

This test was failing because the error code didn't match. The specific error code isn't important.

Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is fine, but does this mean the message is different?

@gaearon
Copy link
Collaborator

gaearon commented Jan 31, 2017

I would assume it means it is crashing for a different reason.

Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we figure out why this is different?

The exact error code isn't important.
@acdlite
Copy link
Collaborator Author

acdlite commented Feb 2, 2017

It was because the message was slightly different, which I fixed in 2afcf6e

I figured loosening the invariant made sense, anyway.

Although after rebasing, now I'm having a different problem where the error code transform isn't picking up that invariant, but works for other invariants in the same file. Weird.

@acdlite
Copy link
Collaborator Author

acdlite commented Feb 2, 2017

Ah, the message is the same as Stack but the template string passed to invariant is different. I'll change.

Also, we haven't run gulp react:extract-errors in a while. Should we automate that?

@acdlite
Copy link
Collaborator Author

acdlite commented Feb 2, 2017

I guess it happens before release so it's not a big deal.

(Or does it...?)

@gaearon
Copy link
Collaborator

gaearon commented Feb 2, 2017

Yea, it's in the release guide so shouldn't be necessary.

The production error code system depends on the exact template string
that is passed to `invariant`. This changes the Fiber templates to
match the Stack ones.
@acdlite
Copy link
Collaborator Author

acdlite commented Feb 2, 2017

@spicyj Going to assume this is good to merge now that we know why the codes were different (they're the same now), since you approved it earlier.

@acdlite acdlite merged commit 57d2d6d into facebook:master Feb 2, 2017
@sophiebits
Copy link
Collaborator

I don't think we should loosen this unnecessarily – even if format changes are okay, problems building the URL or similar could surface and be caught by the old test. Can we revert this? (If I rescind my acceptance then please don't consider a PR accepted.)

@acdlite
Copy link
Collaborator Author

acdlite commented Feb 2, 2017

Ok I'll revert it and open a separate PR without the change to the test.

@acdlite
Copy link
Collaborator Author

acdlite commented Feb 2, 2017

And sorry, I didn't intentionally ignore your review... I thought it was ambiguous whether you had "rescinded" it or not, since there were two reviews, hence my comment :D

@sophiebits
Copy link
Collaborator

Latest review wins. :)

acdlite added a commit that referenced this pull request Feb 2, 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