react-rails icon indicating copy to clipboard operation
react-rails copied to clipboard

Expose all built-in options for `getConstructor`

Open hibachrach opened this issue 5 years ago • 5 comments

Summary

This enables us to easily override getConstructor to not use global fallback, avoiding the all-consuming try...catch.

Other Information

Here's the context:

Quoting https://github.com/reactjs/react-rails/issues/264#issuecomment-552326663

Regarding Encountered error "#<ExecJS::ProgramError: Invariant Violation: Element type is invalid: ...:

I think one of the core issues is that module lookup uses try...catch. While the errors are logged to the console shim, that typically doesn't help as a later error (such as the invariant violation) will lead to a fatal error (triggering a 500). If that could be refactored to be a bit more intentional based on environment (instead of just reacting based on exceptions, or at the very least, throwing if the caught exception isn't very specific)

hibachrach avatar Jan 10 '20 08:01 hibachrach

@HarrisonB can you rebase this on master?

justin808 avatar Aug 17 '22 10:08 justin808

Sure! I'll hopefully get to it in the next week or two

hibachrach avatar Aug 17 '22 22:08 hibachrach

@justin808 updated!

hibachrach avatar Aug 26 '22 04:08 hibachrach

@HarrisonB can you confirm that all unit tests run locally?

Currently, we don't have CI working. We need to move to GH Actions.

justin808 avatar Aug 26 '22 05:08 justin808

Having some trouble--See https://github.com/reactjs/react-rails/pull/1198#issuecomment-1229123819

hibachrach avatar Aug 30 '22 02:08 hibachrach

@hibachrach is this ready for merge?

justin808 avatar Sep 27 '22 10:09 justin808

yes, though we should have it trigger CI to make sure it passes. is there a way to do that on your end?

hibachrach avatar Sep 29 '22 21:09 hibachrach

@hibachrach did you rebase on master where the github actions are defined?

justin808 avatar Sep 30 '22 09:09 justin808

Tests are now passing!

hibachrach avatar Oct 01 '22 06:10 hibachrach

@justin808 The PR is good to go if we have no review comments.

alkesh26 avatar Oct 07 '22 11:10 alkesh26

Thanks @hibachrach!

justin808 avatar Oct 19 '22 19:10 justin808