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

What is the reasoning behind remove-empty/import? #65

Closed
EvgenyOrekhov opened this issue Jun 28, 2021 · 4 comments
Closed

What is the reasoning behind remove-empty/import? #65

EvgenyOrekhov opened this issue Jun 28, 2021 · 4 comments
Labels
plugin-remove-empty question Further information is requested

Comments

@EvgenyOrekhov
Copy link

Using "empty" imports for side effects is a fairly common practice:

// jest.setup.js
import "@testing-library/jest-dom/extend-expect.js";

What is the reasoning behind removing them by default?

@coderaiser
Copy link
Owner

coderaiser commented Jun 28, 2021

Because it’s not common practice for built-ins, there is a lot ways to import module like fs or any module from the wild:

import ‘fs’;
import ‘mock-import’;

That has no sense. Also with help of ignore option any most used this way modules can be listed and excluded.

Do you have a list of such modules?

@coderaiser coderaiser added plugin-remove-empty question Further information is requested labels Jun 28, 2021
@EvgenyOrekhov
Copy link
Author

EvgenyOrekhov commented Jun 28, 2021

Not sure about built-ins, but empty imports are used pretty often by 3rd-party libraries.

Thanks for clarifications, I decided to disable remove-empty/import (along with import/no-unassigned-import) in eslint-config-hardcore: https://github.com/EvgenyOrekhov/eslint-config-hardcore/releases/tag/v19.10.0.

@coderaiser
Copy link
Owner

coderaiser commented Jun 28, 2021

I see that jest-dom suggest to use:

import '@testing-library/jest-dom'

Let's look what is inside src/extend-expect.js:

import * as extensions from './matchers'
expect.extend(extensions)

You can use similar approach and this is is more obvious to understand what is going on here without any documentation. supertape behave in a similar way:

const {extend} = require('supertape');
const test = extend({
    transform: (operator) => (a, b, message = 'should transform') => {
        const {is, output} = operator.equal(a + 1, b - 1);
        return {
            is,
            output,
        };
    },
});

test('assertion', (t) => {
    t.transform(1, 3);
    t.end();
});

With no pollution of global scope. Many task runners adds global variables but I don't think this is a good practice for example js-based Test Anything Protocol runners can work without separated utility you just execute your test with:

node test.js

And it just works as any other script, with jest, mocha etc you can't do such a trick.
Anyways jest is very good task runner with a lot of features, supertape is the complete oppositeЖ the realm of minimalism :).

Also worth mention that: Explicit is better than implicit according to Zen of Python. Also such a way is violates Principle of least astonishment: when you see empty import first thing you want to do is delete it (if it's not css in React Component of course).

@EvgenyOrekhov
Copy link
Author

I agree that it would be better to avoid unassigned imports, but unfortunately many libraries recommend using unassigned imports in the official docs, and it just becomes annoying to see false-positive warnings from import/no-unassigned-import and remove-empty/import.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin-remove-empty question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants