🐛 Ignore side-effect imports when organising imports #817

remojansen · 2023-11-21T14:38:21Z

Environment information

CLI:
  Version:                      1.3.3
  Color support:                true

Platform:
  CPU Architecture:             aarch64
  OS:                           macos

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

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

Workspace:
  Open Documents:               0

What happened?

Using biome format --write ./ in a project.

Input:

import '@projectx/theme/src/normalize.css';
import './index.css';
import '@projectx/theme/src/button.css';
import '@projectx/theme/src/col-3-section.css';
import '@projectx/theme/src/col-50x50-section.css';
import '@projectx/theme/src/col-full-section.css';
import '@projectx/theme/src/onetrust.css';
import '@projectx/theme/src/rich-text-accordion.css';
import '@projectx/theme/src/rich-text-table.css';
import '@projectx/theme/src/side.css';
import '@projectx/theme/src/summary.css';
import '@projectx/theme/src/utils.css';
import '@projectx/theme/src/component-ui-form.css';
import '@projectx/theme/src/main-app.css';
import { LayoutInternal } from './LayoutInternal';
import { LoginStatus } from '@projectx/constants-common';
import { ErrorHandling } from '@projectx/container-error-handler';
import { CustomError } from './CustomError';
import React from 'react';

Actual Output (Import sorting):

import { LoginStatus } from '@projectx/constants-common';
import { ErrorHandling } from '@projectx/container-error-handler';
import '@projectx/theme/src/button.css';
import '@projectx/theme/src/col-3-section.css';
import '@projectx/theme/src/col-50x50-section.css';
import '@projectx/theme/src/col-full-section.css';
import '@projectx/theme/src/component-ui-form.css';
import '@projectx/theme/src/main-app.css';
import '@projectx/theme/src/normalize.css';
import '@projectx/theme/src/onetrust.css';
import '@projectx/theme/src/rich-text-accordion.css';
import '@projectx/theme/src/rich-text-table.css';
import '@projectx/theme/src/side.css';
import '@projectx/theme/src/summary.css';
import '@projectx/theme/src/utils.css';
import React from 'react';
import { CustomError } from './CustomError';
import { LayoutInternal } from './LayoutInternal';
import './index.css';

Changing the order of CSS imports breaks the application. CSS imports should not be sorted.

Playground demo

Expected result

Expected Output (Import sorting):

import '@projectx/theme/src/normalize.css';
import './index.css';
import '@projectx/theme/src/button.css';
import '@projectx/theme/src/col-3-section.css';
import '@projectx/theme/src/col-50x50-section.css';
import '@projectx/theme/src/col-full-section.css';
import '@projectx/theme/src/onetrust.css';
import '@projectx/theme/src/rich-text-accordion.css';
import '@projectx/theme/src/rich-text-table.css';
import '@projectx/theme/src/side.css';
import '@projectx/theme/src/summary.css';
import '@projectx/theme/src/utils.css';
import '@projectx/theme/src/component-ui-form.css';
import '@projectx/theme/src/main-app.css';
import { LoginStatus } from '@projectx/constants-common';
import { ErrorHandling } from '@projectx/container-error-handler';
import React from 'react';
import { CustomError } from './CustomError';
import { LayoutInternal } from './LayoutInternal';

Confirmed as a bug in #807

Code of Conduct

I agree to follow Biome's Code of Conduct

The text was updated successfully, but these errors were encountered:

remojansen · 2023-11-21T14:55:29Z

I've been looking at the source code, and here are my initial thoughts.

The file extension .css is not relevant.
Side effects imports (imports without from) should not be organized
Side effects imports are identifiable in the AST as JSImportBareClause

If this is correct, my approach would be to treat each side effects import as an import group. The changes should be mostly contained within/crates/biome_js_analyze/src/assists/correctness/organize_imports.rs.

The bug fix will change how import groups are identified:

From line breaks only (current implementation)
To line breaks or side effect imports.
Each side effect import will become a single-item import group. The following test cases should pass:

Test case A (Input)

import '@projectx/theme/src/normalize.css';
import './index.css';
import '@projectx/theme/src/button.css';
import '@projectx/theme/src/col-3-section.css';
import '@projectx/theme/src/col-50x50-section.css';
import '@projectx/theme/src/col-full-section.css';
import '@projectx/theme/src/onetrust.css';
import '@projectx/theme/src/rich-text-accordion.css';
import '@projectx/theme/src/rich-text-table.css';
import '@projectx/theme/src/side.css';
import '@projectx/theme/src/summary.css';
import '@projectx/theme/src/utils.css';
import '@projectx/theme/src/component-ui-form.css';
import '@projectx/theme/src/main-app.css';
import { LayoutInternal } from './LayoutInternal';
import { LoginStatus } from '@projectx/constants-common';
import { ErrorHandling } from '@projectx/container-error-handler';
import { CustomError } from './CustomError';
import React from 'react';

Test case A (Output)

import '@projectx/theme/src/normalize.css';
import './index.css';
import '@projectx/theme/src/button.css';
import '@projectx/theme/src/col-3-section.css';
import '@projectx/theme/src/col-50x50-section.css';
import '@projectx/theme/src/col-full-section.css';
import '@projectx/theme/src/onetrust.css';
import '@projectx/theme/src/rich-text-accordion.css';
import '@projectx/theme/src/rich-text-table.css';
import '@projectx/theme/src/side.css';
import '@projectx/theme/src/summary.css';
import '@projectx/theme/src/utils.css';
import '@projectx/theme/src/component-ui-form.css';
import '@projectx/theme/src/main-app.css';
import { LoginStatus } from '@projectx/constants-common';
import { ErrorHandling } from '@projectx/container-error-handler';
import React from 'react';
import { CustomError } from './CustomError';
import { LayoutInternal } from './LayoutInternal';

Test case B (Input)

import '@projectx/theme/src/normalize.css';
import './index.css';
import '@projectx/theme/src/button.css';
import '@projectx/theme/src/col-3-section.css';
import '@projectx/theme/src/col-50x50-section.css';
import '@projectx/theme/src/col-full-section.css';
import '@projectx/theme/src/onetrust.css';
import '@projectx/theme/src/rich-text-accordion.css';
import '@projectx/theme/src/rich-text-table.css';
import { LoginStatus } from '@projectx/constants-common';
import { ErrorHandling } from '@projectx/container-error-handler';
import '@projectx/theme/src/side.css';
import '@projectx/theme/src/summary.css';
import '@projectx/theme/src/utils.css';
import '@projectx/theme/src/component-ui-form.css';
import '@projectx/theme/src/main-app.css';
import { LayoutInternal } from './LayoutInternal';
import { CustomError } from './CustomError';
import React from 'react';

Test case B (Output)

import '@projectx/theme/src/normalize.css';
import './index.css';
import '@projectx/theme/src/button.css';
import '@projectx/theme/src/col-3-section.css';
import '@projectx/theme/src/col-50x50-section.css';
import '@projectx/theme/src/col-full-section.css';
import '@projectx/theme/src/onetrust.css';
import '@projectx/theme/src/rich-text-accordion.css';
import '@projectx/theme/src/rich-text-table.css';
import { LoginStatus } from '@projectx/constants-common';
import { ErrorHandling } from '@projectx/container-error-handler';
import '@projectx/theme/src/side.css';
import '@projectx/theme/src/summary.css';
import '@projectx/theme/src/utils.css';
import '@projectx/theme/src/component-ui-form.css';
import '@projectx/theme/src/main-app.css';
import React from 'react';
import { CustomError } from './CustomError';
import { LayoutInternal } from './LayoutInternal';

Please let me know if you are happy with this approach.

Conaclos · 2023-11-21T15:04:27Z

@remojansen

Otherwise, we could extract all side-efect imports and put them together at the start.
This could change the output of Test B to:

import '@projectx/theme/src/normalize.css';
import './index.css';
import '@projectx/theme/src/button.css';
import '@projectx/theme/src/col-3-section.css';
import '@projectx/theme/src/col-50x50-section.css';
import '@projectx/theme/src/col-full-section.css';
import '@projectx/theme/src/onetrust.css';
import '@projectx/theme/src/rich-text-accordion.css';
import '@projectx/theme/src/rich-text-table.css';
import '@projectx/theme/src/side.css';
import '@projectx/theme/src/summary.css';
import '@projectx/theme/src/utils.css';
import '@projectx/theme/src/component-ui-form.css';
import '@projectx/theme/src/main-app.css';
import { LoginStatus } from '@projectx/constants-common';
import { ErrorHandling } from '@projectx/container-error-handler';
import React from 'react';
import { CustomError } from './CustomError';
import { LayoutInternal } from './LayoutInternal';

Also, a some users requested new behaviors for the organize-imports feature. How your proposal could fit with their request?

remojansen · 2023-11-21T16:26:55Z

Hi @Conaclos,

My approach has two steps:

Identify all side effect imports (JSImport with JSImportBareClause nodes)
Make each side effect import a group

Moving all side effect imports to the top will be slightly more complex it would introduce one more additional step:

Move the groups with side effects imports to the front of the group list (without sorting)

The final step happens in both approaches:

Let sorting happen (within the groups as it is right now)

However, my main concern is not the additional step. My main concern is that maybe there are weird cases in which it could cause issues. I'm not sure about it (I'm not an expert on the semantics of JS modules).

Both approaches should not be an issue in connection with #645:

The new feature blankLineBetweenGroups will add a line after a group. This feature would be compatible with both my proposed and your proposed approach, so it should not be an issue. However, I think this should be implemented in a separate PR, not as part of this bug fix.
The new feature importGroups allows users to customise the sorting order. As @ematipico mentioned, this would increase complexity quite a lot. It is up to you guys if you want to go in this direction I would personally not do this unless is requested by a large part of the user community.

ematipico · 2023-11-21T16:46:13Z

I would expect the proposed - hence not decided - options to adapt to the side effects logic, not the other way around. I think we should fix the bug by pretending that those proposed options aren't there yet.

Side-effects handling is more important than some user-facing options.

remojansen · 2023-11-21T16:56:48Z

Yes, I would like to fix the bug without adding any new options to the config.

I propose to treat side-effect imports as import groups with one import per group. By doing this, I will have to change the logic that identifies the groups, but I will not have to change the logic that sorts the groups. If you want to add "move side effect imports to the top", I would also have to change the sorting logic, but it should not be a massive change. I will simply move the groups with JSImportBareClause to the front before sorting occurs. I just need to know if @ematipico approves this proposed implementation.

Alternatively, I could not touch the groups and modify only the sorting; from my experience, implementing it would be more complex. Maybe you know a better alternative approach, but again, from my inexperience, these are the two approaches that I thought about.

remojansen · 2023-11-21T17:00:30Z

I think moving the side effects to the top might not be a good idea. I found this and it makes sense to me:

Since it is hard to know which side effect imports affect which other imports, I would suggest the following solution: treat a side effect import as a boundary that splits the imports into two groups, the imports before the the side effect import and the imports after the side effect import. - Source

This re-inforces the idea of treating side-effect imports as import groups.

ematipico · 2023-11-21T18:42:30Z

It makes sense to me!

Conaclos · 2023-11-21T23:30:48Z

You convinced me :)

moogus · 2023-11-30T11:44:24Z

Hiya, I wondered if there is any update on this?

ematipico · 2023-11-30T19:16:45Z

@moogus From this discussion

I am happy to try to work on a bug fix over the Christmas break. It will be my first time working with Rust and contributing to Biome. I will probably need some help, but I'm feeling brave. I wanted to give Rust a go, so this could be my perfect excuse.

moogus · 2023-12-01T10:37:26Z

@ematipico Amazing, thank you

ematipico · 2024-01-05T14:16:41Z

@remojansen are there any updates?

remojansen · 2024-01-05T15:48:37Z

Hi, I have it done. I am just solving testing issues now. I have a question about something that has been impacted. There is a test case for comments in imports:

source: crates/biome_js_analyze/tests/spec_tests.rs
expression: comments.js

Input

// leading b
import b, {
    B, // dangling b
} from 'b'; // trailing b
// leading a
import a, {
    A, // dangling a
} from 'a'; // trailing a

// leading d
import d, {
    D, // dangling d
} from 'd'; // trailing d
// leading c
import c, {
    C, // dangling c
} from 'c'; // trailing c
// leading eof

Actions

@@ -1,18 +1,18 @@
+// leading a
+import a, {
+    A, // dangling a
+} from 'a'; // trailing a
 // leading b
 import b, {
     B, // dangling b
 } from 'b'; // trailing b
-// leading a
-import a, {
-    A, // dangling a
-} from 'a'; // trailing a
 
+// leading c
+import c, {
+    C, // dangling c
+} from 'c'; // trailing c
 // leading d
 import d, {
     D, // dangling d
 } from 'd'; // trailing d
-// leading c
-import c, {
-    C, // dangling c
-} from 'c'; // trailing c
 // leading eof

I broke this test but as soon as I find a fix I should be good to go.

chekrd · 2024-02-12T15:40:28Z

In the meantime, as I haven't found any other way to disable a formatter for a module (biome-ignore does not work for this case), the only workaround is adding the module name to the ignore list:

// Biome config
"files": {
  "ignore": [
    "moduleName",
  ]
}

which is a bummer when you need to ignore all index.ts(x) files as we do, because it disables all formating rules, not just importing. Or is there a way to disable just a single formatting rule for a module?

ematipico · 2024-02-12T18:06:40Z

@chekrd

We document it in our docs

chekrd · 2024-02-12T18:29:21Z

I see, thanks. Yes, adding a new line with an explanation comment should work as a temporal workaround. You are doing a great job answering that fast! 🙂

ematipico · 2024-02-13T08:42:32Z

I take for granted that @remojansen doesn't have time to fix the bug. Help here will be appreciated

Fixes: #817

Conaclos added L-JavaScript Language: JavaScript and super languages S-Bug-confirmed Status: report has been confirmed as a valid bug labels Nov 21, 2023

ematipico assigned remojansen Nov 30, 2023

ematipico assigned ematipico and unassigned remojansen Feb 12, 2024

ematipico removed their assignment Feb 13, 2024

ematipico added S-Help-wanted Status: you're familiar with the code base and want to help the project A-Analyzer Area: analyzer labels Feb 13, 2024

ematipico self-assigned this Apr 8, 2024

Conaclos self-assigned this Apr 18, 2024

Conaclos mentioned this issue Apr 21, 2024

📎 Integrate lint/useGroupedTypeImport in organizeImports #551

Closed

Conaclos added a commit that referenced this issue Apr 22, 2024

fix(organizeImport): ignorer side effect imports

8f7fc02

Fixes: #817

Conaclos added a commit that referenced this issue Apr 22, 2024

fix(organizeImport): ignorer side effect imports

3dbd546

Fixes: #817

Conaclos mentioned this issue Apr 22, 2024

fix(organizeImport): ignore side effect imports #2563

Merged

Conaclos closed this as completed in #2563 Apr 22, 2024

DeltaV93 mentioned this issue Jun 19, 2024

[Snyk] Upgrade @biomejs/biome from 1.7.2 to 1.7.3 TechByChoice/tbc-members-core-ui#228

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Ignore side-effect imports when organising imports #817

🐛 Ignore side-effect imports when organising imports #817

remojansen commented Nov 21, 2023

remojansen commented Nov 21, 2023

Conaclos commented Nov 21, 2023

remojansen commented Nov 21, 2023

ematipico commented Nov 21, 2023

remojansen commented Nov 21, 2023

remojansen commented Nov 21, 2023 •

edited

Loading

ematipico commented Nov 21, 2023

Conaclos commented Nov 21, 2023

moogus commented Nov 30, 2023

ematipico commented Nov 30, 2023

moogus commented Dec 1, 2023

ematipico commented Jan 5, 2024

remojansen commented Jan 5, 2024

chekrd commented Feb 12, 2024 •

edited

Loading

ematipico commented Feb 12, 2024

chekrd commented Feb 12, 2024

ematipico commented Feb 13, 2024

🐛 Ignore side-effect imports when organising imports #817

🐛 Ignore side-effect imports when organising imports #817

Comments

remojansen commented Nov 21, 2023

Environment information

What happened?

Expected result

Code of Conduct

remojansen commented Nov 21, 2023

Test case A (Input)

Test case A (Output)

Test case B (Input)

Test case B (Output)

Conaclos commented Nov 21, 2023

remojansen commented Nov 21, 2023

ematipico commented Nov 21, 2023

remojansen commented Nov 21, 2023

remojansen commented Nov 21, 2023 • edited Loading

ematipico commented Nov 21, 2023

Conaclos commented Nov 21, 2023

moogus commented Nov 30, 2023

ematipico commented Nov 30, 2023

moogus commented Dec 1, 2023

ematipico commented Jan 5, 2024

remojansen commented Jan 5, 2024

source: crates/biome_js_analyze/tests/spec_tests.rs expression: comments.js

Input

Actions

chekrd commented Feb 12, 2024 • edited Loading

ematipico commented Feb 12, 2024

chekrd commented Feb 12, 2024

ematipico commented Feb 13, 2024

remojansen commented Nov 21, 2023 •

edited

Loading

source: crates/biome_js_analyze/tests/spec_tests.rs
expression: comments.js

chekrd commented Feb 12, 2024 •

edited

Loading