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

fix(parser): correctly parse record and tuple tokens #13418

Merged
merged 3 commits into from Jun 9, 2021

Conversation

fedeci
Copy link
Member

@fedeci fedeci commented Jun 3, 2021

Q                       A
Fixed Issues? Fixes #13417
Patch: Bug Fix? Y
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

#{ and #[ tokens now hold the correct location.
|] and |} are now finished as tokens and not operators.

@fedeci fedeci added pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Jun 3, 2021
@babel-bot
Copy link
Collaborator

babel-bot commented Jun 3, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/46719/

@fedeci
Copy link
Member Author

fedeci commented Jun 3, 2021

I noted that we don't have token specific fixtures, should we add those?
Moreover #13417 also mentions that value for the most of the tokens is undefined. We may do something like this in the Token constructor:

constructor(state: State) {
    this.type = state.type;
    if (state.value !== undefined) {
      this.value = state.value;
    }
    this.start = state.start;
    this.end = state.end;
    this.loc = new SourceLocation(state.startLoc, state.endLoc);
  }

but no tests are throwing, since they are not writing value in the output if undefined.

@fisker
Copy link
Contributor

fisker commented Jun 3, 2021

Thanks for the quick fix, have you checked |} token as well?

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jun 3, 2021

I noted that we don't have token specific fixtures, should we add those?

@fedeci You can add a new babel-parser/test/tokens.js file with normal describe-it-expect Jest tests, or a new subfolder of test/fixtures.

@JLHwung
Copy link
Contributor

JLHwung commented Jun 3, 2021

We need similar fix for [| and {|.

@KFlash
Copy link

KFlash commented Jun 3, 2021

That token constructor code is super slow :) Why there is a need for a new SourceLocation. Can't this be a plain object?
And regarding the "missing" value. All object should keep the same shape to avoid hidden classes issue and perf regression in V8.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 4, 2021

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 3d15e07:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@fedeci fedeci changed the title fix(parser): correctly parse token location for #{ and #[ fix(parser): correctly parse record and tuple tokens Jun 8, 2021
@KFlash
Copy link

KFlash commented Jun 9, 2021

@JLHwung No plans to fix the performance issues?

@JLHwung
Copy link
Contributor

JLHwung commented Jun 9, 2021

@KFlash I am aware of the performance issue here, but fixing the performance falls out of the scope of this PR and thus should be addressed in another PR.

@JLHwung JLHwung merged commit a64d08c into babel:main Jun 9, 2021
@fedeci fedeci deleted the fix-record-tuple-tokenizer branch June 9, 2021 04:00
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Sep 8, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: #{ token in Record don't have correct location info & value
6 participants