Skip to content
This repository has been 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
merged 11 commits into from
Aug 7, 2023
Merged

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

merged 11 commits into from
Aug 7, 2023

Conversation

nissy-dev
Copy link
Contributor

@nissy-dev nissy-dev commented Jul 29, 2023 •
edited

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
Copy link

netlify bot commented Jul 29, 2023 •
edited

✅ 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
Copy link

github-actions bot commented Jul 29, 2023 •
edited

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
Copy link
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
crates/rome_js_parser/src/syntax/stmt.rs Outdated Show resolved Hide resolved
crates/rome_js_parser/src/syntax/stmt.rs Outdated
@@ -1107,7 +1158,7 @@ pub(super) enum VariableDeclarationParent {
Clause,
}

/// 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
Copy link
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
Copy link
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
Copy link
Contributor

@arendjr arendjr left a comment

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!

crates/rome_js_analyze/src/analyzers/style/use_single_var_declarator.rs
@@ -52,7 +52,11 @@ impl Rule for UseSingleVarDeclarator {
semicolon_token,
} = node.as_fields();

let JsVariableDeclarationFields { kind, declarators } = declaration.ok()?.as_fields();
let JsVariableDeclarationFields {
await_token: _,
Copy link
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

Copy link
Contributor Author

@nissy-dev nissy-dev Aug 6, 2023 •
edited

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.

crates/rome_js_parser/src/syntax/stmt.rs Outdated Show resolved Hide resolved
crates/rome_js_parser/src/syntax/stmt.rs Show resolved Hide resolved
@nissy-dev
Copy link
Contributor Author

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

@nissy-dev
Copy link
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
Copy link
Contributor Author

nissy-dev commented Aug 6, 2023 •
edited

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
Copy link
Contributor

@arendjr arendjr left a comment •
edited

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
Copy link
Contributor Author

The CI failure is related to PlayForm/Compress#163

@ematipico ematipico merged commit d25b06d into main Aug 7, 2023
18 of 19 checks passed
@ematipico ematipico deleted the feat-explicit-resource-management branch August 7, 2023 10:49
@Conaclos
Copy link
Contributor

Conaclos commented Aug 7, 2023

Such a good work!

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

@arendjr arendjr arendjr approved these changes

@ematipico ematipico ematipico approved these changes

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.

None yet
4 participants
@nissy-dev @ematipico @Conaclos @arendjr