[eslint] Don't crash on multiple @babel/parser copies#13274
[eslint] Don't crash on multiple @babel/parser copies#13274nicolo-ribaudo merged 4 commits intobabel:mainfrom
@babel/parser copies#13274Conversation
| `../fixtures/duplicated-babel-parser/a.js`, | ||
| ), | ||
| ]), | ||
| ).not.toThrow(); |
There was a problem hiding this comment.
This throws without the fix (it's the example from #12985 (comment))
|
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 5601f32:
|
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/45897/ |
JLHwung
left a comment
There was a problem hiding this comment.
Can we check whether we have different copies of @babel/parser when loading covertTokens? Then we can use the slower string match only when we know a fast token match check is not feasible.
|
When the objects would be equal, the strings are the same (I don't mean that they are v8 internally compares them by pointer, so it's as fast as comparing the objects:
If we want to improve perf, a good thing to do might be replacing the EDIT: Since the strings are interned (because they are short and directly present in the source code), also the |
|
Did you mean https://github.com/v8/v8/blob/dc712da548c7fb433caed56af9a021d964952728/src/objects/objects.cc#L821 ? The token is a plain object while I agree that they are probably neglectable compared to parse/estraverse. So I am good on this PR. |
kaicataldo
left a comment
There was a problem hiding this comment.
One question about increasing test coverage but otherwise LGTM!
| import { tokTypes as tt } from "@babel/core"; | ||
| import { tokTypes } from "@babel/core"; | ||
|
|
||
| const tl = (process.env.BABEL_8_BREAKING |
There was a problem hiding this comment.
Do we want to try to figure out a way to test this? Would it work to just manually set process.env.BABEL_8_BREAKING in our tests?
There was a problem hiding this comment.
Yeah, it's tested by the "Test Babel 8 breaking changes" job (even if it doesn't show up in codecov).
When running locally, you can use BABEL_8_BREAKING=true make test.
Reading the v8 code (but it's the first time I do it!), I think that when comparing interned strings it also compares just the memory address (the second of my links), so it should be equally fast (it does three checks: pointer + first is interned + second is interned, but it still doesn't need to iterate the string). |
This PR replaces object ref comparisons with string comparisons.