un-ts / eslint-plugin-import-x

`eslint-plugin-import-x` is a fork of `eslint-plugin-import` that aims to provide a more performant and more lightweight version of the original plugin.
https://npm.im/eslint-plugin-import-x
MIT License
428 stars 20 forks source link

Add option for no-rename-default to exclude some modules #136

Open meteorlxy opened 2 months ago

meteorlxy commented 2 months ago

Some 3rd party modules may minify the released files. It would be helpful to add an option to exclude those modules manually.

For example, markdown-it-anchor:

image

Any thoughts? @SukkaW

SukkaW commented 2 months ago

Some 3rd party modules may minify the released files.

That's a fair point, which is why this rule is included in the "warning" presets.

As for markdown-it-anchor, it does export{b as default}; after modification.

It would be helpful to add an option to exclude those modules manually.

IMHO, it is not a good idea to manually configure for every module that publishes minified dist.

SukkaW commented 2 months ago

@meteorlxy How about we proceed this way: Ignore all defaultExportedName instances where the length is less than or equal to three.

Three characters provide 199,888 (52 62 62) possible mangled exports, which is more than sufficient for most modules as they won't have that many exports in one file or chunk. If they do, eslint-disable would suffice.

I chose three characters because two characters only yield 3,224 (52 * 62) possible mangled exports, a number that some icon libraries might easily surpass.

However, this introduces another issue: there are popular libraries, such as xo and mem, which have names shorter than three characters, and we want to validate them too.

What do you think?

Zamiell commented 2 months ago

Rather than hardcode length, is it possible to detect minified modules? Could we make the assumption that a module is minified if there are no newlines in it?

SukkaW commented 2 months ago

Rather than hardcode length, is it possible to detect minified modules? Could we make the assumption that a module is minified if there are no newlines in it?

What about comments then? webpack retains license comments by default even when minified is enabled. And webpack won't hoist those comments either.

meteorlxy commented 2 months ago

Ignore all defaultExportedName instances where the length is less than or equal to three.

Thus, ignoring exports based on char length it not ideal.


Could we make the assumption that a module is minified if there are no newlines in it?

webpack retains license comments by default even when minified is enabled. And webpack won't hoist those comments either.

We'd better not rely on those assumptions:

  1. Minifying a file does not necessarily change the name of the default export. It might minify other parts and keep the variables name. That's all configurable by the minifier.
  2. Users may manually write one-line module. It's not common but definitely possible.
  3. The comments are also optional. It could be kept or removed. All configurable, too.

IMHO, it is not a good idea to manually configure for every module that publishes minified dist.

In fact, minified module is only one of the use case of the rule options.

It's always better to provide an option for users to configure this rule in a fine-grained manner:

For example, it could be nice to have something like this:

{
  'import/no-rename-default': [
    'warn',
    {
      modules: [
        ['markdown-it-anchor', { ignore: true }], // ignore default export name checking
        ['markdown-it-anchor', { override: 'markdownItAnchor' }], // override default export name to markdownItAnchor
      ],
    },
  ],
}

Don't think it too verbose. You can't imagine how users want to configure rules for large-scale projects.

SukkaW commented 2 months ago

Don't think it too verbose. You can't imagine how users want to configure rules for large-scale projects.

It is a common practice to publish minified dist to npm. I personally published more than 20 npm packages that have their dist minified. Explicit configuring all of them is definitely not an option.

SukkaW commented 1 month ago

@Zamiell @meteorlxy I have an idea. We could add an option here, and we could only apply this rule for internal modules, not dependencies.

Zamiell commented 1 month ago

@SukkaW I think this rule should have a whitelist in exactly the same way as the whitelist that I added here: https://github.com/un-ts/eslint-plugin-import-x/pull/142

However, with that said, in my PR here, you said that we should hold off and that it was related to a whitelist. I do not think that is true. Regardless of whether a module is internal or external, if the export is specifically named "default" or "index", then it is obviously a mistake for no-rename-default to be flagging the code. I think it is common sense that the author of the export does not intend for "default" or "index" to be a name that the end-user is matching in the downstream code!

Thus I think for now we should hard whitelist these two names, and then offer consumers of the ESLint rule the option for further customization.

SukkaW commented 1 month ago

I think it is common sense that the author of the export does not intend for "default" or "index" to be a name that the end-user is matching in the downstream code!

The same would apply to any minified or mangled name. It doesn't mean we have to add every word combination with a length of less than three to the whitelist.

Also, the last thing I want is for someone to claim they intentionally exported index instead of any other name.

Zamiell commented 1 month ago

The same would apply to any minified or mangled name. It doesn't mean we have to add every word combination with a length of less than three to the whitelist. Also, the last thing I want is for someone to claim they intentionally exported index instead of any other name.

This is just my opinion, but I could see some codebases having 3 letter exports that are not minified. Like "car" or "dog" or "cat". Thus I think it is dangerous to whitelist all 3 letter names.

However, I think that "default" and "index" belong in a separate category - it is much less likely that those would ever be intended by the author. Why? Because both of these words have specific meanings in JavaScript. default is a JavaScript keyword, so TypeScript does not even allow it as a variable name! index is a bit different since it is actually allowed as a variable name, but it is still a "special" name such that both Node.js and browsers will automatically look for a file with that name. Thus naming anything index in the JavaScript ecosystem is not idiomatic.

AndreaPontrandolfo commented 3 weeks ago

@Zamiell @meteorlxy I have an idea. We could add an option here, and we could only apply this rule for internal modules, not dependencies.

I support this. This option alone would be a huge improvement for the rule. Meaning: an option to only lint internal modules, ignore NPM modules.

Ideally it should also support monorepos setups though: in a monorepo, internal package A imports internal package B. But B comes from workspace:^ not npm, so it should be treated as an internal module, not an external package, and thus B should be linted by this rule (i hope i made it clear what i mean to say).

SukkaW commented 3 weeks ago

internal package A imports internal package B. But B comes from workspace:^ not npm, so it should be treated as an internal module

I am against that. You are already using monorepo and separating A and B into two packages. Now A and B are isolated from each other, they behave like npm packages from the package's perspective of view.

And if we ignore packages from the current monorepo, what happens if you mangle A's exports (just like many other npm packages) during the build step?

AndreaPontrandolfo commented 3 weeks ago

internal package A imports internal package B. But B comes from workspace:^ not npm, so it should be treated as an internal module

I am against that. You are already using monorepo and separating A and B into two packages. Now A and B are isolated from each other, they behave like npm packages from the package's perspective of view.

My point was that while developing in a monorepo, the internal packages will not be minified (it depends on how you setup your environment of course, but that would be counterproductive and wasteful), so they can be linted by the rule.

SukkaW commented 1 week ago

the internal packages will not be minified

This is not true. You'd most likely (and should) import the dist of other packages (instead of their src). So it is actually possible (and most likely) to encounter mangled exports even inside the monorepo.