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

Side-effecting imports should be treated as a special case #505

Open
callumlocke opened this issue Aug 22, 2016 · 5 comments
Open

Side-effecting imports should be treated as a special case #505

callumlocke opened this issue Aug 22, 2016 · 5 comments

Comments

@callumlocke
Copy link

callumlocke commented Aug 22, 2016

By side-effecting imports, I mean when you import something just for its side effects, without assigning a variable. (Not sure what the correct term is.)

For example:

import './lib/env'; // side-effects!

import path from 'path';
import execa from 'execa';

Side-effecting imports do things like reading config files and setting up the environment (in ways that might impact the way other modules initialise), so it usually makes sense for them to go first, regardless of whether they're absolute/relative. But the imports-first rule doesn't consider this, and complains about the above snippet, saying "Absolute imports should come before relative imports."

I think it would make sense for the imports-first rule to make an exception for side-effecting imports.

(Sorry if this already exists as an option and I'm just missing it.)

@benmosher
Copy link
Member

Interesting. I would expect anything for this to end up in the order rule, though it currently is 100% based on path, so this would mean adding another orthogonal grouping dimension.

Gut reaction is conflicting; I think it makes sense to separate+group side-effect imports, but it also adds a decent amount of complexity to add it as a dimension to the order rule.

@sompylasar
Copy link
Contributor

Refs #520

@sompylasar
Copy link
Contributor

Note: not just imports without variable binding like import './lib/env'; can cause side-effects.

Example 1: export + implicit side-effect

// ./FooComponent.js

import styles from './FooComponent.scss';  
// ^^^ style-loader causes side-effects by inserting styles into the page
//     + css-modules exports the class names of the styles

export default function FooComponent() {}

Example 2: chained import + indirect side-effects

// ./FooComponent.js

import './FooComponent.css';
// style-loader causes side-effects by inserting styles into the page

export default function FooComponent() {}
// ./BarComponent.js

import FooComponent from './FooComponent';
// ^^^ this import causes side-effects indirectly by importing a side-effectful module

export default function BarComponent() {}

@benmosher
Copy link
Member

FWIW, I am assuming you're not suggesting we inspect the module content for side effects. The most I would expect from the order rule would be to put imports with explicit side effects (or rather: no imported names) somewhere special.

@jfmengels
Copy link
Collaborator

@benmosher The order rule simply ignores imports with explicit side-effects. I agree that we should do the same for imports-first. For those with implicit side-effects, an // eslint-disable-line is the way to go.

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

No branches or pull requests

4 participants