Add rule to enforce default import naming #1143

mic4ael · 2018-07-24T06:41:37Z

This rule enforces a specific naming for default imports.

coveralls · 2018-07-24T06:57:02Z

Coverage increased (+0.05%) to 97.959% when pulling f48590d on mic4ael:rename-default-import into fef718c on benmosher:master.

ljharb

This isn't quite what was discussed in #1041.

docs/rules/rename-default-import.md

mic4ael · 2018-07-24T08:26:44Z

Updated ;)

mic4ael · 2018-07-30T13:22:46Z

So I guess it is not gonna be accepted unless I make it conform to the propositions listed in the linked issue?

ThiefMaster · 2018-08-03T17:20:31Z

How is this related to #1041? That issue seems to be about enforcing the same name a library uses to export its default with (e.g. export default function foo() {}) while this one is simply about enforcing a name chosen by whoever configures eslint.

src/rules/rename-default-import.js

mic4ael · 2018-08-30T08:27:01Z

Any update on this?

docs/rules/rename-default-import.md

mic4ael · 2018-09-19T07:05:18Z

Any update on this one?

docs/rules/rename-default-import.md

src/rules/rename-default-import.js

mic4ael · 2018-09-24T06:42:35Z

Updated. Let me know what you think.

tests/src/rules/rename-default-import.js

mic4ael · 2018-09-28T10:15:05Z

Let me know what you think now

mic4ael · 2018-10-04T07:56:47Z

Any update on this? ;)

src/rules/rename-default-import.js

mic4ael · 2018-10-16T06:33:55Z

Updated once again.

ljharb

What happens with import * as Something from 'prop-types'?

docs/rules/rename-default-import.md

src/rules/rename-default-import.js

CHANGELOG.md

ljharb · 2020-01-07T00:12:02Z

tests/src/rules/rename-default-import.js

+    test({
+      code: `import {default as propTypes, foo} from 'prop-types';`,
+      options: [{'prop-types': 'PropTypes'}],
+      output: `import {default as PropTypes, foo} from 'prop-types';`,


i'm a little concerned with making this autofixable, if you won't be updating all the other places ijn the file where this variable is referenced (and it might not be safe to update it, if it's used in something like const obj = { PropTypes };).

As far as I can tell this will update all the references apart from the exported identifiers. I am getting confused. Isn't it something that we agreed on?

@ljharb can you please elaborate on this comment since I think it does what you said.

Can we include test cases that have lots of references to the imported thing, so the autofix output can demonstrate that it updates too?

ljharb · 2020-01-07T00:12:21Z

docs/rules/rename-default-import.md

+  "rules": {
+    "import/rename-default-import": [
+      "warn", {
+        "prop-types": "PropTypes", // key: name of the module, value: desired binding for default import


what validates that the RHS of this object is a valid JS identifier?

At the moment it is only the user of the plugin who should be aware what they are doing. I could do something like eval(`(() => {var ${mapping}})()`) or new Function(`var ${mapping}`)

Even though it is not the safest way to check for this..

is checking that even important? sounds like a case of garbage-in, garbage-out if people put invalid stuff there...

I prefer my projects not to ever emit garbage :-)

If we decide to validate the user-provided identifier I would like to find out an easy way to do it. Because so far I couldn't find a simple and secure solution.

Hi @mic4ael
Sorry for reincarnating this thread after such a long time. My take on this would be to parse either var ${mapping} or ${mapping} using espree - a module also used internally by eslint for parsing code.

In case of ${mapping}:
https://astexplorer.net/#/gist/94e57078a7e95dbf910a262c645dd3ea/b82ba422cd8e946bee02a1209795a1bbaa9be225

The parsed AST should consist of exactly one ExpressionStatement having an Identifier set as its expression. Everything else (or an exception) is treated as an incorrect mapping.

In case of var ${mapping}:
https://astexplorer.net/#/gist/40823431b92aaf01426b255e4ad7163f/24a943d0b759cdf8aff897dfa1c5dcba48cb3847

The parsed AST should consist of exactly one VariableDeclaration, which has its init set to null and its id set to an Identifier.

That way you can be sure mapping is no garbage.

ljharb · 2020-06-05T22:45:04Z

@Uko the PR is waiting for a response from @mic4ael.

@mic4ael i apologize for the very long delay in getting you a review, but it's been almost as long since I've provided one :-) are you still interested in updating this PR?

mic4ael · 2020-06-06T09:45:37Z

@Uko the PR is waiting for a response from @mic4ael.

@mic4ael i apologize for the very long delay in getting you a review, but it's been almost as long since I've provided one :-) are you still interested in updating this PR?

hey @ljharb sorry for the delay, I will update my PR soon ;)

ljharb

Thanks! I made a few tweaks and added a test case (which is unfortunately failing).

I'm not convinced on the rule name, though - rename-default-import is a bit weird; this seems more like enforce-import-name. It also kind of seems like the same rule should be preventing named imports from being renamed with as?

mic4ael · 2020-09-15T09:29:44Z

Thanks! I made a few tweaks and added a test case (which is unfortunately failing).

I'm not convinced on the rule name, though - rename-default-import is a bit weird; this seems more like enforce-import-name. It also kind of seems like the same rule should be preventing named imports from being renamed with as?

Ok, I will rename it and will look into the suggestion you made.

mic4ael · 2020-09-15T13:07:22Z

Also, as to this:

It also kind of seems like the same rule should be preventing named imports from being renamed with as

Can you please provide an example of what exactly you mean by that?

mic4ael · 2020-09-29T11:40:15Z

hey @ljharb, can you please take a quick look at my responses? I would like to clarify some of the issues and (maybe) finalize this PR.

benmosher · 2020-10-19T12:32:20Z

FWIW, I agree with @ljharb that the rule's name sounds like it might enforce any import name, not just the default, but I don't know that there is much value in the latter and I think the config could be made smarter if that enhancement is desired later, e.g. "prop-types": { "default": "PropTypes", "allow-renames": false | ["x", "y"] | ... } -- so I am fine keeping enforce-import-name but I'll defer to @ljharb.

OMG, guys. You are so slow.

Hard to argue with this 😅 since I'm generally MIA and @ljharb is a person dedicated to keeping it clean and understands the cost of adding code that you're not prepared to maintain, which I really appreciate.

Also, there is always patch-package in a pinch!

phaux · 2020-10-19T23:06:46Z

I just looked at this rule source code and it looks like it doesn't do at all what was described in the original #1041 feature request? It was supposed to enforce the name in the import statement, but it should go to the imported file and look at the export name and enforce that.

Right now it only enforces the names which you define in the options and you have to manually list all the modules with default exports and also find out what are the names of these default exports. All this information is already available: for external packages it's in their .d.ts file and for relative imports it's in the source code of the imported file.

I'm 100% sure it's possible to implement because VSCode can suggest autoimports based on the names of default exports in other files. Also I'm pretty sure that TSLint has a similar rule.

benmosher · 2020-10-27T15:08:53Z

Ah yep, good point @phaux. also @ThiefMaster and @ljharb before you, heh. 😅 I should have re-read the source issue before chiming in at the end. (also: I didn't read enough of the following discussion to know if I'm about to just restate discussion from there 😬)

@mic4ael: is it important for your use that the rule config allow you to lint for a name that is not the original module's default export's name?

Properly solving #1041 will necessitate the original module code is available but that is typically the expected case for this plugin, and the resolver sub-plugins implement that side of it anyway (e.g. the TS resolver plugin knows where to find .d.ts files or TS source, node resolver checks package.json, etc.).

Also consequently, if the default export is some nameless expression, this rule would not apply.

Less config == better config, IMO. I'd be fine with allowing overrides if absolutely necessary, but not allowing them and just always reading the source for the default export's true name would be simpler.

ThiefMaster · 2020-10-27T16:05:14Z

Also consequently, if the default export is some nameless expression, this rule would not apply.

I don't think this is helpful... as a user of a library you have no influence on that. I don't know if e.g. prop-types has a named default export... (we want it to be importet consistently as propTypes and not PropTypes)

benmosher · 2020-10-28T13:24:54Z

Okay, so there is some desire to allow overrides then. That would support unnamed exports, false modules (e.g. CommonJS), and renaming.

Still seems like you'd want the default behavior to pick up the module's original name if it's available, though?

mic4ael force-pushed the rename-default-import branch from 9ea1200 to 072305c Compare July 24, 2018 07:00

ljharb requested changes Jul 24, 2018

View reviewed changes

docs/rules/rename-default-import.md Outdated Show resolved Hide resolved

docs/rules/rename-default-import.md Outdated Show resolved Hide resolved

mic4ael force-pushed the rename-default-import branch 2 times, most recently from af337e7 to db0623e Compare July 24, 2018 08:26

ljharb changed the title ~~Add rule to enforce default import aliases~~ Add rule to enforce default import naming Jul 24, 2018

ThiefMaster reviewed Aug 10, 2018

View reviewed changes

src/rules/rename-default-import.js Outdated Show resolved Hide resolved

mic4ael force-pushed the rename-default-import branch from db0623e to a138319 Compare August 10, 2018 13:31

ljharb reviewed Sep 2, 2018

View reviewed changes

docs/rules/rename-default-import.md Outdated Show resolved Hide resolved

docs/rules/rename-default-import.md Outdated Show resolved Hide resolved

mic4ael force-pushed the rename-default-import branch from a138319 to 2b5dde9 Compare September 3, 2018 09:06

ljharb requested changes Sep 19, 2018

View reviewed changes

ThiefMaster reviewed Sep 19, 2018

View reviewed changes

src/rules/rename-default-import.js Outdated Show resolved Hide resolved

mic4ael force-pushed the rename-default-import branch from 2b5dde9 to 11361eb Compare September 24, 2018 06:41

ljharb requested changes Sep 25, 2018

View reviewed changes

tests/src/rules/rename-default-import.js Outdated Show resolved Hide resolved

tests/src/rules/rename-default-import.js Outdated Show resolved Hide resolved

mic4ael force-pushed the rename-default-import branch 2 times, most recently from f0f7cef to 7f2fcef Compare September 26, 2018 11:56

mic4ael force-pushed the rename-default-import branch from 7f2fcef to 73969b6 Compare October 15, 2018 10:04

ThiefMaster reviewed Oct 15, 2018

View reviewed changes

src/rules/rename-default-import.js Outdated Show resolved Hide resolved

src/rules/rename-default-import.js Outdated Show resolved Hide resolved

src/rules/rename-default-import.js Outdated Show resolved Hide resolved

mic4ael force-pushed the rename-default-import branch 2 times, most recently from 62c11b4 to 0da6220 Compare October 16, 2018 06:33

ljharb reviewed Oct 16, 2018

View reviewed changes

docs/rules/rename-default-import.md Outdated Show resolved Hide resolved

src/rules/rename-default-import.js Outdated Show resolved Hide resolved

src/rules/rename-default-import.js Outdated Show resolved Hide resolved

src/rules/rename-default-import.js Outdated Show resolved Hide resolved

mic4ael force-pushed the rename-default-import branch from 68a276f to 56d9aaf Compare October 21, 2019 12:06

pscelzamelo approved these changes Jan 6, 2020

View reviewed changes

ljharb requested changes Jan 7, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

mic4ael force-pushed the rename-default-import branch from 56d9aaf to 6a6b827 Compare May 12, 2020 06:47

mic4ael force-pushed the rename-default-import branch 5 times, most recently from 64e823f to 6194ac0 Compare June 7, 2020 11:32

ljharb force-pushed the rename-default-import branch from 6194ac0 to f579ea7 Compare June 7, 2020 22:51

ljharb requested changes Jun 7, 2020

View reviewed changes

soniadeveloper mentioned this pull request Jul 24, 2020

[repo] enforce classnames import name format via eslint rule carbon-design-system/carbon-addons-iot-react#1325

Open

mic4ael force-pushed the rename-default-import branch from f579ea7 to 126ad82 Compare September 15, 2020 08:36

Add rule to enforce default import aliases

f48590d

mic4ael force-pushed the rename-default-import branch from 126ad82 to f48590d Compare September 15, 2020 13:08

CarinaChenot mentioned this pull request Dec 13, 2020

Proposal: default-import-match-filename #325

Open

ljharb marked this pull request as draft August 29, 2022 21:13

grahamlangford mentioned this pull request Jun 30, 2023

Restrict imports of classnames to cx pixiebrix/eslint-config-pixiebrix#64

Closed

This comment was marked as spam.

Sign in to view

Add rule to enforce default import naming #1143

Are you sure you want to change the base?

Add rule to enforce default import naming #1143

Conversation

mic4ael commented Jul 24, 2018 • edited Loading

coveralls commented Jul 24, 2018 • edited Loading

ljharb left a comment

Choose a reason for hiding this comment

mic4ael commented Jul 24, 2018

mic4ael commented Jul 30, 2018

ThiefMaster commented Aug 3, 2018

mic4ael commented Aug 30, 2018

mic4ael commented Sep 19, 2018

mic4ael commented Sep 24, 2018

mic4ael commented Sep 28, 2018

mic4ael commented Oct 4, 2018

mic4ael commented Oct 16, 2018

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

ljharb commented Jun 5, 2020

mic4ael commented Jun 6, 2020

ljharb left a comment

Choose a reason for hiding this comment

mic4ael commented Sep 15, 2020

mic4ael commented Sep 15, 2020

mic4ael commented Sep 29, 2020

benmosher commented Oct 19, 2020 • edited Loading

phaux commented Oct 19, 2020

benmosher commented Oct 27, 2020

ThiefMaster commented Oct 27, 2020

benmosher commented Oct 28, 2020

This comment was marked as spam.

mic4ael commented Jul 24, 2018 •

edited

Loading

coveralls commented Jul 24, 2018 •

edited

Loading

benmosher commented Oct 19, 2020 •

edited

Loading