feat(biome_js_analyze): `useImportExtensions` - add `importMapping` options by drdaemos · Pull Request #3274 · biomejs/biome

drdaemos · 2024-06-24T10:04:29Z

Summary

This is a solution for #2967.

In TypeScript-based projects that use a ES Module import system, useImportExtensions might suggest an incorrect import extension, depending on the project configuration and runtime used. TypeScript requires you to manually specify .js or .mjs extensions (see microsoft/TypeScript#49083 (comment)) even in .ts or .mts files.

This change allows to specify importMappings option for useImportExtensions to override the default Biome suggestions.

Test Plan

I have added a separate spec invalidWithImportMappings.ts to verify that the custom import mappings take precedence over the default logic and apply correct mapping to both JS/TS and *SX components.

For the following linter configuration:

{
    "rules": {
        "nursery": {
			"useImportExtensions": {
				"level": "error",
				"options": {
            	    "importMappings": [
                	    {
                      		"fileExt": ["ts"],
                       		"importExt": "js",
                       		"componentImportExt": "jsx"
                       	}
                    ]
                }
			}
		}
	}
}

All .ts files will attempt to either import .js or .jsx files, depending on the filename.

…describe JSON schema

kibertoad · 2024-06-24T10:48:05Z

+			"required": ["componentImportExt", "fileExt", "importExt"],
+			"properties": {
+				"componentImportExt": {
+					"description": "Extension that should be used for component file imports",


what is the difference between component file and import in this context?

Rule defines component files as ones starting with an upper-case letter - I guess it is common for React / Vue components - hence the ability to use a different jsx / tsx extension.

This was a stop gap solution until Biome has infra to check filesystem. Bit unfortunate we need to expose that option, but not sure more general solution is possible.

Though maybe if user picks .js we could auto pick .jsx for those component imports, and likewise .ts -> .tsx? Or would that be too much magic?

This is a more or less advanced configuration - personally I would prefer to just have an explicit configuration.

codspeed · 2024-06-24T11:36:54Z

CodSpeed Performance Report

Merging #3274 will not alter performance

_{Comparing drdaemos:feat/import_mappings_option (29673a4) with drdaemos:feat/import_mappings_option (5603764)}

Summary

✅ 90 untouched benchmarks

minht11 · 2024-06-24T11:47:49Z

+    ///
+    /// ### Options
+    ///
+    /// Use the options to specify correct import extensions for your project based on the linted


Should we mention difference between browser/deno bun bundler/pure tsc somewhere and when you would use such option?

Oh, that's a rabbit hole which I don't feel comfortable to describe fully and conclusively. I would be glad to hear suggestions on what we can mention here.

I agree with @drdaemos here. There's a whole world here, and I don't think the rule should discuss it. However, if there are some useful resources, we could add them in the future.

minht11 · 2024-06-24T11:58:17Z

+    /// List of inspected file extensions that are subject to this mapping
+    pub file_ext: Vec<String>,
+    /// Extension that should be used for module imports
+    pub import_ext: String,


These extensions are only used for suggestions. I can't really see them used for anything else in a future in this rule (might be wrong).

So should it be named suggested_import_extension?

What do you mean? Isn't the whole idea behind this PR to fix the auto-fix functionality, which currently uses the extension that is wrong for Node.js environment?

This rule only checks for imports without extensions. Biome does not have infra yet, to check for filesystem. This PR won't fix wrong extensions. It will only suggested user selected extension when there isn't one.

I can see checking for wrong extensions added in a future, so my naming change can be ignored, though I imagine that won't be easy to get right in all cases.

OK, so to clarify: biome check --fix --unsafe currently does add missing extension when there isn't one at all. That is expected, and after this change the user-specified extension will be applied. Is that correct?

What you are saying is that if e. g. original file is "file.ts", and the import in code is "file.rtf", then Biome will say that it's perfectly fine, right?

Yes to both. import { named } from './file.rtf' is valid import as far as this rule sees it.

minht11 · 2024-06-24T12:11:07Z

+			"required": ["componentImportExt", "fileExt", "importExt"],
+			"properties": {
+				"componentImportExt": {
+					"description": "Extension that should be used for component file imports",


This was a stop gap solution until Biome has infra to check filesystem. Bit unfortunate we need to expose that option, but not sure more general solution is possible.

Though maybe if user picks .js we could auto pick .jsx for those component imports, and likewise .ts -> .tsx? Or would that be too much magic?

Co-authored-by: Justinas Delinda <8914032+minht11@users.noreply.github.com>

Conaclos · 2024-06-24T13:17:33Z

We could use a hash map for the mapping:

{
  "rules": {
    "nursery": {
      "useImportExtensions": {
        "level": "error",
        "options": {
          "suggestedExtensions": {
            "ts": {
              "import": "js",
              "component": "jsx"
            }
          }
        }
      }
    }
  }
}

kibertoad · 2024-06-24T14:30:33Z

@Conaclos So should we keep the explicit mapping? Based on your comment on the issue, I thought you prefer a simple binary toggle.

Conaclos · 2024-06-24T14:58:56Z

So should we keep the explicit mapping? Based on your comment on the issue, I thought you prefer a simple binary toggle.

I am waiting other feedback before deciding.
If we keep the map we could switch to what I propose here.

drdaemos · 2024-06-24T20:51:33Z

I have updated the options format based on @Conaclos' suggestion. I guess having an explicit mapping option is the best bet until Biome can detect actual imports used in runtime or replicates the whole logic of how ES modules should be imported depending on the project configuration and used bundlers.

ematipico · 2024-06-25T08:30:34Z

+    ///             "ts": {
+    ///                 "import": "js",
+    ///                 "component": "jsx"
+    ///             }


I believe we should be more descriptive here. We should explain the example, and we should explain what import is and how it affects the code action, and we should explain what component is and how component affects the code action.

Usually, we follow this format:

Talk about the options

Explain the example e.g. "If you want to do this ... then you should define this and that", "If you have this other situation ... then you should use define this option in this way"

Code block of the example

To give you better feedback. I am reading this for the first time, and I still don't understand how these options work. For example "ts" as a key isn't explained. Is it an input? An output?

I think we could add some rationale. We could inform users that it is a temporary workaround. Ideally we should query the extension of the imported file and choose an extension based on that.

I have updated the description and tried to provide a deeper explanation for the reasoning and way to use that in a project. Do you mind taking another look?

ematipico · 2024-06-25T08:32:35Z

+#[serde(rename_all = "camelCase", deny_unknown_fields)]
+pub struct SuggestedExtensionMapping {
+    /// Extension that should be used for module imports
+    pub import: String,


I would call this module, because that's exactly what it is.

Indeed, module seems better.

ematipico · 2024-06-25T08:36:37Z

+#[derive(Clone, Debug, Default, Deserializable, Deserialize, Serialize, Eq, PartialEq)]
+#[cfg_attr(feature = "schemars", derive(JsonSchema))]
+#[serde(rename_all = "camelCase", deny_unknown_fields)]
+pub struct UseImportExtensionsOptions {


What do you think about a configuration like this:

{ "suggestedExtensions": [ { "inputExtensions": ["ts", "tsx"], "import": "js", "component": "jsx" } ] }

That's what I had intially before this comment: #3274 (comment).

I'm not really sure which option is better, both seem sufficient to case for handling backend code + tsc

I have no big preference. However, using a hash map is easier to query in the code. I think we should not aim for perfection here, because the mapping should be a temporary workaround. We should not spend too much time on that.

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

kibertoad · 2024-06-28T08:16:24Z

@ematipico @Conaclos Is anything missing on the PR?

ematipico · 2024-06-28T08:54:27Z

@ematipico @Conaclos Is anything missing on the PR?

Some of my comments haven't been addressed. Mainly documentation.

# Conflicts: # CHANGELOG.md # crates/biome_js_analyze/src/lint/nursery/use_import_extensions.rs

kibertoad · 2024-07-01T09:53:42Z

@ematipico Documentation was updated now, I believe

ematipico

Looks good, just one nit around docs and then we can merge it

ematipico · 2024-07-01T10:17:10Z

+    /// Mainly, this is a temporary workaround that allows Biome to propose correct import extensions
+    /// for TypeScript projects that use ES Modules. TypeScript requires you to specify imports to
+    /// the actual files used in runtime: `.js` or `.mjs` (see more here: https://github.com/microsoft/TypeScript/issues/49083#issuecomment-1435399267).


I would move this paragraph after the code block of the options. Also, add a :::caution block, so the docs will render this with a warning block:

/// :::caution /// Mainly, this is a temporary ... /// :::

Reordered and added the warning block around.

drdaemos added 4 commits June 24, 2024 02:08

feat(biome_js_analyze): useImportExtensions - import mapping options

b8c5c88

feat(biome_js_analyze): useImportExtensions - add snapshot tests and …

d35463e

…describe JSON schema

fix: convert CRLF to LF in snap

185eeab

fix: run generators and fix lint issues

1e67bf4

github-actions Bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Jun 24, 2024

drdaemos and others added 2 commits June 24, 2024 14:04

Merge branch 'main' into feat/import_mappings_option

e09f8da

chore: update CHANGELOG.md

c460f5b

github-actions Bot added the A-Changelog Area: changelog label Jun 24, 2024

drdaemos marked this pull request as ready for review June 24, 2024 10:08

drdaemos changed the title ~~feat(biome_js_analyze): useImportExtensions - import mapping options~~ feat(biome_js_analyze): useImportExtensions - add importMapping options Jun 24, 2024

chore: add descriptions to added options

b9b47f7

kibertoad reviewed Jun 24, 2024

View reviewed changes

minht11 reviewed Jun 24, 2024

View reviewed changes

fix: use minht11 suggestion

87b6831

Co-authored-by: Justinas Delinda <8914032+minht11@users.noreply.github.com>

Conaclos mentioned this pull request Jun 24, 2024

useImportExtensions should use .js extension for TypeScript imports #2967

Closed

1 task done

drdaemos and others added 2 commits June 25, 2024 00:45

chore: change importMappings vec to suggestedExtensions map

304ac95

Merge branch 'main' into feat/import_mappings_option

6a01005

ematipico reviewed Jun 25, 2024

View reviewed changes

drdaemos and others added 3 commits June 25, 2024 16:16

chore: use suggestion

1d44000

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

chore: use suggestion

6206780

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

chore: use suggestion

21d3652

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

chore: update description for suggestedExtensions

aaa06e6

drdaemos added 2 commits June 28, 2024 17:28

Merge branch 'refs/heads/main' into feat/import_mappings_option

fa43525

# Conflicts: # CHANGELOG.md # crates/biome_js_analyze/src/lint/nursery/use_import_extensions.rs

chore: update schemas

4beea05

drdaemos requested review from Conaclos, ematipico and minht11 June 28, 2024 13:36

ematipico approved these changes Jul 1, 2024

View reviewed changes

drdaemos and others added 2 commits July 1, 2024 16:01

chore: update options doc

5603764

format

29673a4

ematipico merged commit 2230e40 into biomejs:main Jul 1, 2024

kibertoad mentioned this pull request Jul 16, 2024

📚 Incorrect documentation for the rule "use-import-extensions" biomejs/website#794

Closed

1 task done

benjamincburns mentioned this pull request Sep 5, 2024

📚 Docs pages reflect unreleased features without flagging them as pre-release biomejs/website#1058

Closed

1 task done

Conversation

drdaemos commented Jun 24, 2024

Summary

Test Plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codspeed Bot commented Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #3274 will not alter performance

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibertoad Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minht11 Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Conaclos commented Jun 24, 2024

Uh oh!

kibertoad commented Jun 24, 2024

Uh oh!

Conaclos commented Jun 24, 2024

Uh oh!

drdaemos commented Jun 24, 2024

Uh oh!

Uh oh!

Uh oh!

ematipico Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibertoad commented Jun 28, 2024

Uh oh!

ematipico commented Jun 28, 2024

Uh oh!

kibertoad commented Jul 1, 2024

Uh oh!

ematipico left a comment

Choose a reason for hiding this comment

codspeed Bot commented Jun 24, 2024 •

edited

Loading

kibertoad Jun 24, 2024 •

edited

Loading

minht11 Jun 24, 2024 •

edited

Loading

ematipico Jun 25, 2024 •

edited

Loading