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

Conversation

nissy-dev
Copy link
Contributor

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

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

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

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.

crates/rome_js_parser/src/syntax/stmt.rs Outdated Show resolved Hide resolved
@@ -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

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!

@@ -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

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

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

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.

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
17 of 18 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.
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
Development

Successfully merging this pull request may close these issues.

4 participants