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

Pass-through JS error to client #521

Merged
merged 4 commits into from
Aug 19, 2016
Merged

Pass-through JS error to client #521

merged 4 commits into from
Aug 19, 2016

Conversation

dzirtusss
Copy link
Contributor

@dzirtusss dzirtusss commented Aug 17, 2016

Pass-through simple version of #485


This change is Reviewable

@dzirtusss
Copy link
Contributor Author

@justin808 @robwise Please look - this is the most minimalist intrusion into error I can imagine. We just update the message and rethrow same error with all same parameters, incl. stacktrace

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.36% when pulling 84824b1 on dzirtusss:js-exception2 into ea95759 on shakacode:master.

@justin808
Copy link
Member

Looking good.

@robwise Take a look.

@samnang @robwise I don't see any worthwhile way to test this. I think we can skip a new test for this change.


Reviewed 1 of 1 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


node_package/src/handleError.js, line 38 [r1] (raw file):

}

export default (options) => {

const handleError = (option) => {
// blah blah
}

export default handleError;


Comments from Reviewable

@robwise
Copy link
Contributor

robwise commented Aug 19, 2016

I think this is a good compromise.

Also, if we are going to be modifying original error messages like this, we should tell the user who is doing it. So message should be like ReactOnRails encountered an error while rendering component: ${name}. Original message: ${e.message}

Or something

@justin808
Copy link
Member

@dzirtusss Let's apply Rob's suggestion and call it good!

@dzirtusss
Copy link
Contributor Author

Done. What about handleGeneratorFunctionIssue? Keep?

@justin808
Copy link
Member

:lgtm:

(Yes keep)


Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@justin808
Copy link
Member

I'll merge once CI passes!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.36% when pulling 7d7dddb on dzirtusss:js-exception2 into ea95759 on shakacode:master.

@justin808
Copy link
Member

@dzirtusss Can you please update the CHANGELOG.md.

@justin808 justin808 merged commit 859ba1b into shakacode:master Aug 19, 2016
@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.36% when pulling c98e25b on dzirtusss:js-exception2 into ea95759 on shakacode:master.

@squaresurf
Copy link

👏

@dzirtusss dzirtusss deleted the js-exception2 branch August 23, 2016 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants