🐛 Ignore side-effect imports when organising imports

[remojansen]

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 https://github.com/biomejs/biome/discussions/807

Code of Conduct

• [X] I agree to follow Biome's Code of Conduct

[remojansen]

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

1. The file extension .css is not relevant.
2. Side effects imports (imports without from) should not be organized
3. 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]

@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]

Hi @Conaclos,

My approach has two steps:

1. Identify all side effect imports (JSImport with JSImportBareClause nodes)
2. 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:

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

The final step happens in both approaches:

5. 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]

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]

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]

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]

It makes sense to me!

[Conaclos]

You convinced me :)

[moogus]

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

[ematipico]

@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]

@ematipico Amazing, thank you

[ematipico]

@remojansen are there any updates?

[remojansen]

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]

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]

@chekrd

We document it in our docs

[chekrd]

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]

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