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

ref: Switch JS parser from rslint to swc #13

Merged
merged 15 commits into from
Oct 17, 2022
Merged

ref: Switch JS parser from rslint to swc #13

merged 15 commits into from
Oct 17, 2022

Conversation

Swatinem
Copy link
Member

@Swatinem Swatinem commented Oct 7, 2022

The rslint parser is unmaintained and has a hard limit on how much code it can parse.

This changes the internal parser so swc, which is maintained and production ready.

Instead of having untyped SyntaxNodes that allow free navigation within the hierarchy, swc is rather based around typed AST nodes and a visitor pattern.

Conversion between the two is mostly straight forward but does involve quite some manual work.

The rslint parser is unmaintained and has a hard limit on how much code
it can parse.

This changes the internal parser so swc, which is maintained and production ready.

Instead of having untyped SyntaxNodes that allow free navigation within
the hierarchy, swc is rather based around typed AST nodes and a visitor pattern.

Conversion between the two is mostly straight forward but does involve quite some manual work.
@Swatinem Swatinem requested a review from a team October 7, 2022 11:37
@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2022

Codecov Report

Merging #13 (233fa92) into master (9bc019c) will decrease coverage by 0.66%.
The diff coverage is 90.22%.

❗ Current head 233fa92 differs from pull request most recent head 5a532a6. Consider uploading reports for the commit 5a532a6 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
- Coverage   89.84%   89.18%   -0.67%     
==========================================
  Files           6        6              
  Lines         394      490      +96     
==========================================
+ Hits          354      437      +83     
- Misses         40       53      +13     

src/scope_name.rs Outdated Show resolved Hide resolved
src/swc.rs Outdated Show resolved Hide resolved
tests/extract.rs Outdated Show resolved Hide resolved
tests/extract.rs Outdated
let scopes = scope_strs(scopes);

let expected = [
(55..79, Some("named_prop".into())),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this one. Feels inconsistent to me. You access it as obj_literal.named_prop in the end.
Also, Chrome is printing both now:

const foo = {
    bar: function baz() {
        throw new Error('wat')
    }
}

foo.bar()

// Uncaught Error: wat
//    at Object.baz [as bar] (<anonymous>:3:15)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that it's the same for Node and any other V8 environment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that if you are giving an explicit name to a function, you would want that to show up.
In your example, baz (the fn name) is also the "primary" thing thats being called out. as bar (the name you call) is only "secondary" information.

@Swatinem Swatinem merged commit cbbf439 into master Oct 17, 2022
@Swatinem Swatinem deleted the ref/swc branch October 17, 2022 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants