Extract, unit test, and improve getLabelFromStackTrace #2534
Conversation
🦋 Changeset detectedLatest commit: 5888e15 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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 5888e15:
|
Codecov Report
|
[skip ci]
|
Amazing work so far, thank you ❤️ I would also take a look at this PR and the included comments: https://github.com/emotion-js/emotion/pull/2136/files . It's fine to continue with our current strategy - just mentioning this for completeness, might give you some new ideas/test cases. Please ping me when this will be ready for review. As far as I can see you are still working on this one. |
|
Thank you!!! The links were very helpful. 😁 I will let you know when the work is done. |
|
@Andarist What are your thoughts on my "Safari is weird" section in the original post? |
This is bizarre 😢 Especially that I don't see what's the difference between both runtimes here - their stack traces have the same structure in all browsers, only the jsx factory name is different.
AFAIK this is not common so I think it's OK to not care about it right now. If we could distinguish this situation in other stack traces reliably to return undefined there then that would be great. But if that's too problematic - then it shouldn't block the work done here. |
|
Btw, I tested this in IE and the new code didn't cause any errors. Of course, the runtime label extraction does not actually work — |
Since we plan to fix this one way or another.
|
Removed the "SSR & Safari" section from the docs since we are planning to fix that one way or another. I also updated the initial comment of the PR to not close the issue about class name mismatch in Safari, since that is still an issue. This should be ready for merge as far as I'm concerned. Thanks. |
| }) | ||
|
|
||
| test('SSR', () => { | ||
| const stackTrace = `Error |
There was a problem hiding this comment.
huh, it's interesting that the same case has produced different-looking stack traces in Chrome and in node, even though they are both using V8 🤔
There was a problem hiding this comment.
It is interesting. It is likely because the Chrome stack trace is from CRA while the SSR one is from Next.js and there are differences in build tooling. Let me know if you want me to redo the Chrome one using the Next.js playground.
| test('Safari', () => { | ||
| const stackTrace = `createEmotionProps@http://localhost:3000/static/js/main.chunk.js:844:49 | ||
| jsxDEV@http://localhost:3000/static/js/main.chunk.js:1126:247 | ||
| MyComponent$9@http://localhost:3000/static/js/main.chunk.js:2274:92 |
There was a problem hiding this comment.
this is somewhat interesting, why the TCO has not been applied here? 🤔
There was a problem hiding this comment.
Yeah it is odd. I can redo this one using the exact code from the comment block if you want. I've noticed that very minor things can affect whether TCO is performed.
| }) | ||
| }) | ||
|
|
||
| test('Chrome: Module.jsx', () => { |
There was a problem hiding this comment.
could you add a note above this block mentioning how one reproduce this situation?
There was a problem hiding this comment.
I never encountered a stack trace containing Module. during all of my testing so I'm not sure how this happens. It's complicated because it could depend on stuff like which version of Webpack is used.
|
Thanks! Please see my latest changes as well as my replies to a few of your comments. |
What:
There were a few of cases where extracting the label from the stack trace did not work correctly.
classNamedid not match #2255.Why:
How:
getLabelFromStackTraceto encapsulate the regex logic.Checklist:
Before changing the implementation of
getLabelByStackTraceAfter changing the implementation
All tests pass.
Safari is weird
This section is now outdated.
It looks like Safari likes to inline function calls, resulting in that function being omitted from the stack trace. In both of the following cases, there will be a "class name mismatch" warning if SSR is used.
typical function component - runtime=classic, Safari: component name does not appear in stack trace. The label should beundefinedin this case.function component within object, Safari: component name is omitted from stack trace. The label should be the parent component's name in this case.const test = { MyComp() { return <div css={{}} /> } }. If this is a corner case, we may not have to worry about it.If either of these are going to be a problem, we could introduce a feature flag that disables label extraction. Label extraction would still be on by default.