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

Bug(typescript-estree): always parse mts/mjs as ESM for non-type-aware parsing #9101

Open
4 tasks done
fisker opened this issue May 15, 2024 · 8 comments · May be fixed by #9121
Open
4 tasks done

Bug(typescript-estree): always parse mts/mjs as ESM for non-type-aware parsing #9101

fisker opened this issue May 15, 2024 · 8 comments · May be fixed by #9121
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working package: typescript-estree Issues related to @typescript-eslint/typescript-estree

Comments

@fisker
Copy link
Contributor

fisker commented May 15, 2024

Before You File a Bug Report Please Confirm You Have Done The Following...

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have searched for related issues and found none that matched my issue.
  • I have read the FAQ and my problem is not listed.

Relevant Package

typescript-estree

Playground Link

No response

Repro Code

import { parse } from "@typescript-eslint/typescript-estree"

const ast = parse('await(1)', {
  project: false,
  filePath: 'foo.mts'
})

console.log(ast.body[0].expression)

ESLint Config

N/A

tsconfig

None

Expected Result

The expression should be AwaitExpression instead of CallExpression.

Actual Result

As described above.

Additional Info

No response

Versions

package version
@typescript-eslint/typescript-estree 7.8.0
TypeScript 5.4.5
node 20
@fisker fisker added bug Something isn't working triage Waiting for maintainers to take a look labels May 15, 2024
@bradzacher bradzacher added package: typescript-estree Issues related to @typescript-eslint/typescript-estree accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for maintainers to take a look labels May 15, 2024
@JoshuaKGoldberg JoshuaKGoldberg changed the title Bug: @typescript-eslint/typescript-estree should support TLA in .mts file Bug: @typescript-eslint/typescript-estree should support top-level await in .mts file May 15, 2024
@JoshuaKGoldberg JoshuaKGoldberg changed the title Bug: @typescript-eslint/typescript-estree should support top-level await in .mts file Bug: @typescript-eslint/typescript-estree should support top-level await (TLA) in .mts file May 15, 2024
@JoshuaKGoldberg
Copy link
Member

Editing the title because it took me a moment to figure this out. 😄

@bradzacher bradzacher changed the title Bug: @typescript-eslint/typescript-estree should support top-level await (TLA) in .mts file Bug(typescript-estree): always parse mts/mjs as ESM for non-type-aware parsing May 15, 2024
@goldentrash
Copy link

goldentrash commented May 19, 2024

I tried to fix this issue! See #9121.
Below is a screenshot of the result.

  • If the file is ESM
    result when ESM

  • If the file is not ESM
    result when not ESM

Copy link

Uh oh! @goldentrash, the image you shared is missing helpful alt text. Check #9101 (comment).

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.

@goldentrash
Copy link

In /tyscript-estree/src/convert.ts/Converter/convertNode, modifying sourceType when it is case SyntaxKind.SourceFile does not make sense, below is the detail.

I tried modifying the sourceFile in convertNode with extension === ts.Extension.Mjs || extension === ts.Extension.Mts, but only the AST root (SourceFile) is changed, not the multiple child Nodes.

It appears that before convertNode is called, the ts AST is fully constructed via ts.createSourceFile and then changed to the estree AST via convertNode.

Screenshot of what I tried

@goldentrash

This comment was marked as outdated.

@goldentrash
Copy link

goldentrash commented May 19, 2024

In /tyscript-estree/src/create-program/createSourceFile.ts/createSourceFile, adjusting impliedNodeFormat, parameter of the ts.createSourceFile, doesn't work, below is the detail.

The Interface annotation says "Controls the format the file is detected as", so I was expecting that, but modifying the impliedNodeFormat didn't achieve the desired result. It seems to be a parameter related to js Emit, not AST construction.

screenshot of what I tried

@fisker

This comment was marked as outdated.

@goldentrash
Copy link

Hi @fisker!

I updated the PR to find a new way to do this because my original suggestion was unacceptable, and the new way I found was to modify setExternalModuleIndicator.

The reason I initially thought it was inappropriate to modify setExternalModuleIndicator was because I was concerned that it would be too difficult to design a new function based on the behavior of the default(but we didn't have to), and that it would be too dependent on external modules.

However, while analyzing the typesciprt code to find a new way, I found that it is very easy to create an externalModuleIndicator that simply returns true unconditionally based on the file extension, and I also found cases where the same thing was done in the typescript internal code.

See PR's comment for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working package: typescript-estree Issues related to @typescript-eslint/typescript-estree
Projects
None yet
4 participants