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

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

Open
meteorlxy opened this issue Aug 28, 2024 · 14 comments
Open

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

meteorlxy opened this issue Aug 28, 2024 · 14 comments

Comments

@meteorlxy
Copy link

meteorlxy commented Aug 28, 2024

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
Copy link
Collaborator

SukkaW commented Aug 28, 2024

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 added a commit to SukkaW/eslint-plugin-import-x that referenced this issue Aug 29, 2024
SukkaW added a commit to SukkaW/eslint-plugin-import-x that referenced this issue Aug 29, 2024
SukkaW added a commit to SukkaW/eslint-plugin-import-x that referenced this issue Aug 29, 2024
SukkaW added a commit to SukkaW/eslint-plugin-import-x that referenced this issue Aug 29, 2024
@SukkaW
Copy link
Collaborator

SukkaW commented Aug 29, 2024

@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
Copy link

Zamiell commented Aug 30, 2024

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
Copy link
Collaborator

SukkaW commented Aug 30, 2024

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
Copy link
Author

meteorlxy commented Aug 31, 2024

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

  • As you mentioned above, name of some famous lib could be shorter than 3. In addition, testing framework may expose it, i18n lib may expose t, etc. It might not be the default export of the module, but it's possible to be the default export of some sub path.
  • Beside 3rd party libs, users may have their own shorter default export within their source code.

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:

  • Ignore some modules
  • Override some modules' default export

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
Copy link
Collaborator

SukkaW commented Aug 31, 2024

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
Copy link
Collaborator

SukkaW commented Sep 22, 2024

@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
Copy link

Zamiell commented Sep 22, 2024

@SukkaW I think this rule should have a whitelist in exactly the same way as the whitelist that I added here: #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
Copy link
Collaborator

SukkaW commented Sep 23, 2024

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
Copy link

Zamiell commented Sep 26, 2024

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
Copy link

@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
Copy link
Collaborator

SukkaW commented Oct 18, 2024

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
Copy link

AndreaPontrandolfo commented Oct 18, 2024

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
Copy link
Collaborator

SukkaW commented Oct 30, 2024

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.

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

4 participants