+
Skip to content
This repository was archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_js_syntax): Allow module syntax in cts files #4317

Merged
merged 5 commits into from
Mar 31, 2023
Merged

fix(rome_js_syntax): Allow module syntax in cts files #4317

merged 5 commits into from
Mar 31, 2023

Conversation

nissy-dev
Copy link
Contributor

Summary

Fixes #3918

This PR is working in progress. Based on #3958, some regression will happen. I will check them and update the PR if necessary.

Test Plan

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented Mar 21, 2023 •
edited
Loading

✅ Deploy Preview for docs-rometools ready!

Name Link
🔨 Latest commit eea5af6
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/6426f834a859ee00082d0e40
😎 Deploy Preview https://deploy-preview-4317--docs-rometools.netlify.app/playground
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions bot added A-Parser Area: parser A-Tooling Area: our own build, development, and release tooling labels Mar 21, 2023
@github-actions
Copy link

github-actions bot commented Mar 21, 2023 •
edited
Loading

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 48647 48647 0
Passed 47582 47582 0
Failed 1065 1065 0
Panics 0 0 0
Coverage 97.81% 97.81% 0.00%

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 6093 6093 0
Passed 1783 1783 0
Failed 4310 4310 0
Panics 0 0 0
Coverage 29.26% 29.26% 0.00%

ts/babel

Test result main count This PR count Difference
Total 639 639 0
Passed 568 568 0
Failed 71 71 0
Panics 0 0 0
Coverage 88.89% 88.89% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 16740 16740 0
Passed 12814 12797 ❌ ⏬ -17
Failed 3926 3943 ❌ ⏫ +17
Panics 0 0 0
Coverage 76.55% 76.45% -0.10%
🔥 Regression (22): 
compiler/moduleNodeDefaultImports.ts
compiler/nodeNextImportModeImplicitIndexResolution2.ts
conformance/externalModules/moduleResolutionWithoutExtension6.ts
conformance/node/nodeModules1.ts
conformance/node/nodeModulesCJSResolvingToESM1_emptyPackageJson.ts
conformance/node/nodeModulesCJSResolvingToESM2_cjsPackageJson.ts
conformance/node/nodeModulesCJSResolvingToESM3_modulePackageJson.ts
conformance/node/nodeModulesCJSResolvingToESM4_noPackageJson.ts
conformance/node/nodeModulesConditionalPackageExports.ts
conformance/node/nodeModulesDeclarationEmitDynamicImportWithPackageExports.ts
conformance/node/nodeModulesDeclarationEmitWithPackageExports.ts
conformance/node/nodeModulesExportsBlocksTypesVersions.ts
conformance/node/nodeModulesForbidenSyntax.ts
conformance/node/nodeModulesImportAssertions.ts
conformance/node/nodeModulesImportResolutionNoCycle.ts
conformance/node/nodeModulesPackageExports.ts
conformance/node/nodeModulesPackageImports.ts
conformance/node/nodeModulesPackagePatternExports.ts
conformance/node/nodeModulesPackagePatternExportsExclude.ts
conformance/node/nodeModulesPackagePatternExportsTrailers.ts
conformance/node/nodePackageSelfName.ts
conformance/node/nodePackageSelfNameScoped.ts
🎉 Fixed (5): 
compiler/nodeNextCjsNamespaceImportDefault1.ts
compiler/nodeNextCjsNamespaceImportDefault2.ts
conformance/node/nodeModulesImportResolutionIntoExport.ts
conformance/node/nodeModulesResolveJsonModule.ts
conformance/node/nodeModulesTypesVersionPackageExports.ts

@nissy-dev nissy-dev marked this pull request as ready for review March 26, 2023 08:48
@nissy-dev
Copy link
Contributor Author

nissy-dev commented Mar 26, 2023 •
edited
Loading

I checked these regressions and all of them was happened in the case that module: node16/nodenext was specified in tsconfig.json.

If module: node16/nodenext was specified in tsconfig.json, ts parser follows Node.js ESM resolution (see: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-7.html). But, Rome doesn't support this resolution logic. Therefore, Rome parser is not reporting errors where it should, resulting in these regressions.

Considering not only linter/formatter but also compiler/bundler, it is better to fix them but I think it is not needed for now. I think there are not so many users to use cts/mts or module: node16/nodenext, so It might not be a problem to fix them until we get a little more feedback from the community.

ematipico
ematipico reviewed Mar 27, 2023
View reviewed changes
ematipico
ematipico approved these changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Could you create a new line in the changelog? https://github.com/rome/tools/blob/main/CONTRIBUTING.md#writing-a-changelog-line

@ematipico ematipico merged commit e72d123 into main Mar 31, 2023
@ematipico ematipico deleted the fix-3918 branch March 31, 2023 18:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ematipico ematipico ematipico approved these changes

Assignees
No one assigned
Labels
A-Parser Area: parser A-Tooling Area: our own build, development, and release tooling
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🐛 Illegal use of an export declaration outside of a module for .cts files
2 participants
@nissy-dev @ematipico
点击 这是indexloc提供的php浏览器服务，不要输入任何密码和下载