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

Removing only side effects imports #3

Open
slavafomin opened this issue Jun 18, 2020 · 13 comments
Open

Removing only side effects imports #3

slavafomin opened this issue Jun 18, 2020 · 13 comments

Comments

@slavafomin
Copy link
Contributor

slavafomin commented Jun 18, 2020

Hello!

Thank you for this great plugin!

However, I want to remove only side-effects imports that looks like this:

import 'something',

but not like these:

  • import { something } from 'something'
  • import something from 'something'

Is it possible?

Thanks!

@jaywcjlove
Copy link
Member

@slavafomin

You should choose prettier and ESLint to solve your problem.

Can meet your needs, but which one is correct to delete?

@slavafomin
Copy link
Contributor Author

slavafomin commented Jun 18, 2020

@jaywcjlove I'm not sure I understand what linting tools have to do with built-time code transformations.

Right now, this plugin would delete imports in all formats, however, I want to delete only imports that looks like this: import 'something', i.e. imports that doesn't import any specific symbols.

@slavafomin
Copy link
Contributor Author

slavafomin commented Jun 18, 2020

Following your notation it would be:

// Input Code
import 'foo';
import { Foo } from 'foo';
import Foo from 'foo';

// Output   ↓ ↓ ↓ ↓ ↓ ↓
import { Foo } from 'foo';
import Foo from 'foo';

jaywcjlove added a commit that referenced this issue Jun 18, 2020
jaywcjlove added a commit that referenced this issue Jun 18, 2020
jaywcjlove added a commit that referenced this issue Jun 18, 2020
@jaywcjlove
Copy link
Member

@slavafomin Upgrade + [email protected]

@jaywcjlove
Copy link
Member

I don't know if this is the feature you want.

@slavafomin
Copy link
Contributor Author

@jaywcjlove wow, that was quick, thank you. I will try it out.

@slavafomin
Copy link
Contributor Author

I've tried to use the following configuration:

    {
      loader: "babel-loader",
      options: {
        plugins: [
          [
            "babel-plugin-transform-remove-imports",
            {
              test: /^core-js\//,
              remove: 'effects',
            },
          ],
        ],
      },
    }

to remove imports that look like: import 'core-js/something';, but I'm getting the following built-time error message: SyntaxError: NodePath has been removed so is read-only, and I'm not sure why. Do you have any thoughts on this?

@slavafomin
Copy link
Contributor Author

@jaywcjlove actually, looking at your implementation I can see a small misunderstanding. I was hoping that remove: 'effects' would work with test option, so both criteria should be met in order for import to be removed.

@slavafomin
Copy link
Contributor Author

If you don't mind I would propose the following API:

    {
      loader: "babel-loader",
      options: {
        plugins: [
          [
            "babel-plugin-transform-remove-imports",
            {
              cases: [ // multiple cases could be specified
                {
                  test: /^core-js\//,
                  remove: "side-effects-only",
                },
                {
                  test: "foo", // string could be used
                  remove: "everything", // default
                },
              ],
            },
          ],
        ],
      },
    }

Such configuration format would allow to specify multiple cases and will allow extensibility in the future by extending the remove option.

A removeAll option could be used instead of cases.

@jaywcjlove
Copy link
Member

When the removeAll option exists, cases does not work? @slavafomin

jaywcjlove added a commit that referenced this issue Jun 20, 2020
jaywcjlove added a commit that referenced this issue Jun 20, 2020
@slavafomin
Copy link
Contributor Author

@jaywcjlove I believe that when removeAll option is used, the usage of cases should throw error to signify that those two options are mutually exclusive.

@jaywcjlove
Copy link
Member

@slavafomin Keep removeAll is compatible with previous versions

@BavlyAbdelmasih
Copy link

is there away to prevent the remove of the import in certain files

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

No branches or pull requests

3 participants