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.

Sign up for GitHub

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

Jump to bottom

🐛 Does not support resolution-mode for typeof import #2115

Open
1 task done
JacobLey opened this issue Mar 17, 2024 · 6 comments · May be fixed by #3462
Open
1 task done

🐛 Does not support resolution-mode for typeof import #2115

JacobLey opened this issue Mar 17, 2024 · 6 comments · May be fixed by #3462
Assignees
@fireairforce
Labels
A-Parser Area: parser L-JavaScript Language: JavaScript and super languages S-Bug-confirmed Status: report has been confirmed as a valid bug S-Help-wanted Status: you're familiar with the code base and want to help the project

Comments

@JacobLey
Copy link

JacobLey commented Mar 17, 2024 •
edited
Loading

Environment information

CLI:
  Version:                      1.6.1
  Color support:                true

Platform:
  CPU Architecture:             aarch64
  OS:                           linux

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v20.11.1"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         unset

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              true
  Organize imports disabled:    false
  VCS disabled:                 false

Workspace:
  Open Documents:               0

What happened?

  1. Declare a type that references an import, using resolution-mode. See example playground: https://www.typescriptlang.org/play?target=99&moduleResolution=99&ts=5.4.2#code/KYDwDg9gTgLgBDAnmYcBiBnOBeBzgQBmcAlgLaSwAUA5IRjQDRwDecA7iTABYBcrcGlGAYIAGwCuMEhAB2AWjIQAJsBr8a5SjBpwAvvoCUQA
  2. Run biome format ., and receive a failure that there is no expected extra parameter to import. See playground https://biomejs.dev/playground/?code=ZQB4AHAAbwByAHQAIAB0AHkAcABlACAARgBzACAAPQAgAHQAeQBwAGUAbwBmACAAaQBtAHAAbwByAHQAKAAnAGYAcwAnACwAIAB7ACAAdwBpAHQAaAA6ACAAewAgACcAcgBlAHMAbwBsAHUAdABpAG8AbgAtAG0AbwBkAGUAJwA6ACAAJwBpAG0AcABvAHIAdAAnACAAfQAgAH0AKQA%3D&jsx=false

Expected result

Biome is able to successfully parse the resolution-mode part of import() and format as usual.

Note resolution-mode is now considered stable in typescript:
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-3.html#stable-support-resolution-mode-in-import-types

Note that prettier also appears to be wrong in this handling, but I think this would count as one of the few cases where biome should diverge from prettier.

This type handling is actually necessary in some cases as well. It is hard to show on playground, but when writing types for a commonjs file that will dynamically import an ESM file, Typescript encourages using the resolution-mode.

my-cjs-file.cts

let nullishEsm: typeof import('my-esm-file') | null = null;
import('my-esm-file').then(val => {
    nullishEsm = val;
}).catch(() => {});

works!

But

let nullishEsm: typeof import('my-esm-file', { with: { 'resolution-mode': 'import' } }) | null = null;
import('my-esm-file').then(val => {
    nullishEsm = val;
}).catch(() => {});

fails with:

The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("my-esm-file")' call instead.

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@Conaclos Conaclos added A-Parser Area: parser S-Bug-confirmed Status: report has been confirmed as a valid bug labels Mar 17, 2024
@Conaclos
Copy link
Member

This is a bug from our parser/grammar. The dynamic import assertion should not be parsed as a js block statement.

@JacobLey
Copy link
Author

For anyone else getting hit by this, I managed to get past the typescript warnings using assert in the import.

import type * as Foo from 'my-esm-file' assert  { 'resolution-mode': 'import' };

// ...

let nullishEsm: typeof Foo | null = null;

Still consider this an issue worth fixing eventually, but definitely possible to work around (and actually appeases eslint rules like consistent-type-imports disallowTypeAnnotations

@ematipico ematipico added S-Help-wanted Status: you're familiar with the code base and want to help the project L-JavaScript Language: JavaScript and super languages labels Mar 30, 2024
@fireairforce
Copy link
Contributor

i want to try this~

@ematipico
Copy link
Member

@fireairforce are there any updates? If not, can we remove yourself and give this issue to someone else?

@fireairforce
Copy link
Contributor

sorry i forget this issue, i will work on this week~

@Conaclos
Copy link
Member

@fireairforce Any update?

@fireairforce fireairforce linked a pull request Jul 17, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fireairforce fireairforce

Labels
A-Parser Area: parser L-JavaScript Language: JavaScript and super languages S-Bug-confirmed Status: report has been confirmed as a valid bug S-Help-wanted Status: you're familiar with the code base and want to help the project
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

(WIP)fix(js-parser): support ts resolution-mode import fireairforce/biome
4 participants
@ematipico @Conaclos @fireairforce @JacobLey