Fix Component Stacks for IE and Native Classes in Safari #18575
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also adjust some expectations. I think the column should ideally be 1 but varies. The Example row is one line off because it throws on the hook but should ideally be the component. Similarly class components with constructors may have the line in the constructor.
We do this by first searching for the first different frame, then find the same frames and then find the first different frame again.
facebook-github-bot
added
CLA Signed
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 5007c88:
|
Details of bundled changes.Comparing: 98d410f...5007c88 react
react-dom
react-art
react-reconciler
ReactDOM: size: 0.0%, gzip: 0.0% React: size: 0.0%, gzip: 0.0% Size changes (experimental) |
Details of bundled changes.Comparing: 98d410f...5007c88 react
react-dom
react-art
Size changes (stable) |
|
There's still one more issue to solve after this. Internet Explorer requires an error to be thrown before it gets a stack. So I'll need to throw instead of just creating the controls. |
gaearon
approved these changes
Apr 11, 2020
Otherwise they don't get a stack frame associated with them in IE.
sebmarkbage
force-pushed
the
proderrorstacks
branch
from
7e8f30d to
b8df429
Compare
|
Ok after these changes it works in IE/Edge. EdgeIE10/11After adjusting the fixtures to avoid ES2015 features. Notably IE8/9 doesn't give you a stack on errors. |
sebmarkbage
force-pushed
the
proderrorstacks
branch
from
672e19b to
28d7c0a
Compare
|
Ok, this turned out to be a bit more code than I had originally hoped. I think we can still shave some bytes off from this implementation but it's still reasonable and much better than people adding meta data to code like displayName or line numbers since that doesn't scale. |
sebmarkbage
changed the title
Errors while generating stacks will bubble to the root. Since this technique is a bit sketchy, we should probably protect against it.
Instead, we pass the prototype as the "this". It's new every time anyway.
sebmarkbage
force-pushed
the
proderrorstacks
branch
from
28d7c0a to
5007c88
Compare
|
Nice! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow up to #18561
It adds more test cases and includes a fix to Safari since Safari adds another stack frame for the construct call. Polyfills might do something similar so this tries to be resilient by skipping over those.
Results from fixtures
Chrome:
Firefox:
Note: Firefox doesn't respect displayName in stack traces.
Safari:
Note: Safari doesn't give us a line number for native classes without an explicit constructor.