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

import/order doesn't sort assets that aren't assigned to a variable (e.g. import './file.css') #1639

Closed
moonray opened this issue Jan 31, 2020 · 9 comments · Fixed by #1990
Closed

Comments

@moonray
Copy link

moonray commented Jan 31, 2020

When sorting imports, any assets that aren't assigned to a variable are ignored, and break/mess with sorting of other imports.

e.g.

import mapboxgl from 'mapbox-gl';

import './file.json';
import './file.css';
import './file.png';
import React from 'react';
import { ToastContainer } from 'react-toastify';

import { ErrorBoundary } from '@app/components';
import { useAccount, useUser } from 'src/state';

import mygif from './file.gif';
import Routes from './Routes';

If I change the above and assign assets to a variable, sorting is applied as expected.

import mapboxgl from 'mapbox-gl';
import React from 'react';
import { ToastContainer } from 'react-toastify';

import { ErrorBoundary } from '@app/components';
import { useAccount, useUser } from 'src/state';

import b from './file.css';
import mygif from './file.gif';
import a from './file.json';
import c from './file.png';
import Routes from './Routes';
@ljharb
Copy link
Member

ljharb commented Jan 31, 2020

Imports without a binding are used for side effects. As such, no bindingless import should ever be moved with an autofixer, and no import-with-a-binding should ever be autofixed such that it runs in a different order with respect to a bindingless import.

@moonray
Copy link
Author

moonray commented Jan 31, 2020

Is there a way to configure this differently?
In tslint this was possible with ordered-imports

I essentially want to enforce grouping all css files together, and all gif|png|jpg|json files together.

@ljharb
Copy link
Member

ljharb commented Jan 31, 2020

It’s not safe to have a rule that does that. Bindingless imports only exist for side effects, and ordering is critical with side effects.

@quezak
Copy link

quezak commented Feb 12, 2020

Your arguments are correct for complicated cases, but it would be nice if you could optionally configure to do something with the side-effect-only imports. I'm currently using a vscode plugin (TypeScript Import Sorter) that puts them on the top, in a separate group, plus a tslint rule that checks that.

IMHO side-effect imports should be used cautiously and only if absolutely necessary, so

  • it's a good thing to explicitly separate them to make them stand out
  • while in some cases the order of side-effect imports may be important, if they need to be mixed in between regular imports, it's a bad thing (and IMHO starts to smell of bad code design)

I'm using side-effect imports only for "global" stuff like reflect-metadata & tsconfig-paths/register on the backend, some jest plugins in tests, or storybook plugins on the frontend. I've just moved from tslint to eslint, and I'd like to keep enforcing grouping side-effect imports together :)

@ljharb
Copy link
Member

ljharb commented Feb 13, 2020

I totally agree that it’s a smell - but a rule that risks making people do the wrong thing, without knowing why, isn’t a good rule.

It might be possible to make this warnable, but never to make it autofixable.

@armano2
Copy link

armano2 commented Feb 13, 2020

maybe suggestions can be used here as they are not automatically applied

https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions

@ljharb
Copy link
Member

ljharb commented Feb 13, 2020

I would be very open to adding suggestions to all non-autofixable warnings in the plugin regardless :-)

@Alphy11
Copy link

Alphy11 commented Feb 12, 2021

@ljharb The fix here is relatively easy if we want to just warn on out of order imports. We are ignoring adding them to the check. The hard part I need an expert to weigh in on is how we want to make the fix. Should we start warning (and not autofixing) on all out of order unassigned imports with default options? Or should I add a new option like warn-on-unassigned-imports?

@ljharb
Copy link
Member

ljharb commented Feb 12, 2021

@Alphy11 it should definitely require a new option, otherwise it's a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants