8000 feat(rome_js_parser, rome_js_formatter): support `using` and `await using` declaration by nissy-dev · Pull Request #4737 · rome/tools · GitHub
[go: up one dir, main page]
Skip to content
This repository was archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_parser, rome_js_formatter): support using and await using declaration - #4737

Merged
ematipico merged 11 commits into
mainfrom
feat-explicit-resource-management
Aug 7, 2023
Merged

feat(rome_js_parser, rome_js_formatter): support using and await using declaration#4737
ematipico merged 11 commits into
mainfrom
feat-explicit-resource-management

Conversation

@nissy-dev
@nissy-dev nissy-dev commented Jul 29, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor

Summary

Fix a part of #4646

This PR is basic support for using and await using declaration.
After merging this PR, I will check if we need to modify more existing lint rules and create another PR for resolving todo tests.

Test Plan

I added some parser tests based on esbuild test cases
ref: https://github.com/evanw/esbuild/blob/af7cfc5d0d5108aae0f9f02447f08761e9b604f2/internal/js_parser/js_parser_test.go#L6090

@netlify
netlify Bot commented Jul 29, 2023 •
edited
Loading
Copy link
Copy Markdown

✅ Deploy Preview for docs-rometools canceled.

Name Link
🔨 Latest commit f4c46bd
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/64cfa5830a95450008926aba

@github-actions github-actions Bot added A-Linter Area: linter A-Formatter Area: formatter L-JavaScript Langauge: JavaScript A-Parser Area: parser A-Tooling Area: our own build, development, and release tooling labels Jul 29, 2023
@nissy-dev nissy-dev changed the title feat: initial support using and await using declaration feat(rome_js_paerser, rome_js_formatter): initial support using and await using declaration Jul 29, 2023
@nissy-dev nissy-dev changed the title feat(rome_js_paerser, rome_js_formatter): initial support using and await using declaration feat(rome_js_parser, rome_js_formatter): initial support using and await using declaration Jul 29, 2023
@github-actions
github-actions Bot commented Jul 29, 2023 •
edited
Loading
Copy link
Copy Markdown

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 48863 48863 0
Passed 47810 47808 ❌ ⏬ -2
Failed 1053 1055 ❌ ⏫ +2
Panics 0 0 0
Coverage 97.84% 97.84% -0.00%
🔥 Regression (2): 
language/statements/labeled/value-await-non-module-escaped.js
language/statements/labeled/value-await-non-module.js

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6212 6212 0
Passed 1764 1764 0
Failed 4448 4448 0
Panics 0 0 0
Coverage 28.40% 28.40% 0.00%

ts/babel

Test result main count This PR count Difference
Total 639 639 0
Passed 573 573 0
Failed 66 66 0
Panics 0 0 0
Coverage 89.67% 89.67% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17224 17224 0
Passed 13121 13121 0
Failed 4103 4103 0
Panics 0 0 0
Coverage 76.18% 76.18% 0.00%

@nissy-dev nissy-dev changed the title feat(rome_js_parser, rome_js_formatter): initial support using and await using declaration feat(rome_js_parser, rome_js_formatter): support using and await using declaration Jul 29, 2023
@nissy-dev
nissy-dev commented Jul 30, 2023
Copy link
Copy Markdown
Contributor Author

I checked if rome handle using and await using declaration correctly in playground, but it didn't handle.
I'm looking into what happened.

ematipico
ematipico reviewed Jul 30, 2023
View reviewed changes
Comment thread crates/rome_js_parser/src/syntax/stmt.rs Resolved
Comment thread crates/rome_js_parser/src/syntax/stmt.rs Outdated
}

/// Parses a variable declaration that consist of a variable kind (`let`, `const` or `var` and a list
/// Parses a variable declaration that consist of a variable kind (`let`, `const`, `using` or `var` and a list
@ematipico ematipico Jul 30, 2023
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The documentation is not correct anymore. The function also checks for await in case of using. We should document this information

@ematipico
ematipico commented Jul 30, 2023
Copy link
Copy Markdown
Contributor

I checked if rome handle using and await using declaration correctly in playground, but it didn't handle. I'm looking into what happened.

You have to update parse_declaration_clause too

arendjr
arendjr reviewed Jul 30, 2023
View reviewed changes
@arendjr arendjr left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great job on the tests!

Comment thread crates/rome_js_analyze/src/analyzers/style/use_single_var_declarator.rs

let JsVariableDeclarationFields { kind, declarators } = declaration.ok()?.as_fields();
let JsVariableDeclarationFields {
await_token: _,
@arendjr arendjr Jul 30, 2023
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this token should be added to the rule’s state, so it can be preserved in the fixer action. I had actually made this change here: main...arendjr:rome-tools:using-proposal#diff-b62c2afc841e799795a6707e4bcdf0f415da3b3183303b3a8507d844f546d455R41

@nissy-dev nissy-dev Aug 6, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing! I will create a separate PR for some fixes in Linter.

Comment thread crates/rome_js_parser/src/syntax/stmt.rs Resolved
Comment thread crates/rome_js_parser/src/syntax/stmt.rs Resolved
@nissy-dev
nissy-dev commented Jul 31, 2023
Copy link
Copy Markdown
Contributor Author

Thank you for your reviews! I will try to fix in a few days.

@nissy-dev
nissy-dev commented Aug 6, 2023
Copy link
Copy Markdown
Contributor Author

You have to update parse_declaration_clause too

I seem "using" cannot be allowed inside a declaration_clause.

https://www.typescriptlang.org/play?ts=5.2.0-beta#code/CYUwxgNghgTiAEBXAzgSwHYHN5QFw-QE8BuAKBAA8AHAexgBck0t4AjeAXiXVADMMQwYkA

@nissy-dev
nissy-dev commented Aug 6, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

I update codes by following comments and this PR is ready for the review again!

arendjr
arendjr approved these changes Aug 7, 2023
View reviewed changes
@arendjr arendjr left a comment •
edited
Loading
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have no further comments, just the tests are red, but that might've been a network issue affecting CI :)

@nissy-dev
nissy-dev commented Aug 7, 2023
Copy link
Copy Markdown
Contributor Author

The CI failure is related to PlayForm/Compress#163

@ematipico
ematipico merged commit d25b06d into main Aug 7, 2023
@ematipico
ematipico deleted the feat-explicit-resource-management branch August 7, 2023 10:49
@Conaclos
Conaclos commented Aug 7, 2023
Copy link
Copy Markdown
Contributor

Such a good work!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@arendjr arendjr arendjr approved these changes
@ematipico ematipico ematipico approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Formatter Area: formatter A-Linter Area: linter A-Parser Area: parser A-Tooling Area: our own build, development, and release tooling L-JavaScript Langauge: JavaScript

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nissy-dev @ematipico @Conaclos @arendjr
0