Correct module real path resolution #1696

paztis · 2020-03-24T13:47:06Z

supports the monorepo correctly
correct the no-extraneous-dependencies usage

…odule real path resolution add real support of isAbsolute (windows + unix support) importType refactoring: use the real resolved package path to check if external of internal, and not the name only like before: in case of monorepo, external modules are not under node_modules due to symlink but still out of the module. correct tests node_modules dependencies to really provide teh package.json like in real usage correct no-extraneous-dependencies rule: get the real name from the resolved package.json. If not aliased imports (alias/react for example) will not be correctly interpreted change path import add real support of isAbsolute (windows + unix support) correct no-extraneous-dependencies rule: get the real name from the resolved package.json. If not aliased imports (alias/react for example) will not be correctly interpreted even externals like "a/file.js" must not use extension. only module names like 'module.js' and '@scope/module.js' are allowed correct bad external definition: must be the folder path under the root of the module. Here the module root is test folder, not cycles folder

coveralls · 2020-03-24T14:17:43Z

Coverage decreased (-2.003%) to 95.795% when pulling d33d69d on paztis:master into efb5f07 on benmosher:master.

coveralls · 2020-03-24T14:17:43Z

Coverage decreased (-2.003%) to 95.795% when pulling d33d69d on paztis:master into efb5f07 on benmosher:master.

coveralls · 2020-03-24T14:17:43Z

Coverage decreased (-2.003%) to 95.795% when pulling d33d69d on paztis:master into efb5f07 on benmosher:master.

coveralls · 2020-03-24T14:17:43Z

Coverage decreased (-0.1%) to 97.868% when pulling 802ce7d on paztis:master into 196d655 on benmosher:master.

paztis · 2020-03-24T16:05:13Z

Not sure to understand why, but I have this CI build that fails only on node v10: https://ci.appveyor.com/project/benmosher/eslint-plugin-import/builds/31684756/job/7lv7l952s32a8qx7

It crash on resolving the .babelrc of the webpack resolver. I don't change this part.
I've try to use a real JSON for babelrc ({"extends": "../../.babelrc"}) but result is the same.

Understand the issue ?

Coverage decrease is because of the fallback on no-extraneous-dependencies we never pass in (it might never apperas in reality, but I keep it for security)

package.json

ljharb · 2020-03-24T21:49:00Z

resolvers/webpack/.babelrc

this was a symlink; why did this file change?

this file import was failing on ApVeillor in case of node v10.
I try to replace its content with

{ "extends": "../../.babelrc" }

to avoid symling problem, but still present, so I rollback it.

I think the extension is a better way as the symlink for local development

This change should still be reverted

I don't know how to commit a sYmlinked file. can you do it after the merge ?

src/core/importType.js

src/core/packagePath.js

paztis · 2020-03-25T00:04:44Z

review changes applied

paztis · 2020-03-28T17:47:51Z

Any new for the PR approval ?

paztis · 2020-04-02T16:45:53Z

No news: start to be impossible to continue without thiese fixes
I will publish my custom version of eslint-plugin-import in case of no news :(

paztis · 2020-04-08T10:10:11Z

any news ?

jimisaacs · 2020-04-14T17:17:21Z

+1

geoffp · 2020-04-15T22:26:35Z

Looking forward to this!

ljharb

Sorry for the delay :-) i've reviewed this from scratch; apologies if you already answered some of these questions

ljharb · 2020-06-05T22:56:54Z

resolvers/webpack/.babelrc

This change should still be reverted

src/core/importType.js

src/rules/extensions.js

src/rules/no-extraneous-dependencies.js

tests/src/core/resolve.js

tests/src/rules/order.js

paztis · 2020-07-29T01:37:42Z

Hi

I review all the changes.

I've only problems with the babelrc file: I cannot commit the original symlink (don't know how to do it). Can you fix it on your side after the merge ?

For the files/symlinked-module change, it is because my module resolution is now really looking at the root package path + folder name, and not only test if the folder name is in the path.
I your unit test, as there's a package.json under the folder tests/src/files, it is considered as the root, and not tests folder as you imagine.
Forced to remove it.
Potentially you may reorganize all the test files under correct folder structure, with correct package.json like in reality.

Please tell me if there's missing things

SanjiKir · 2020-07-31T09:57:34Z

Hey guys, will this PR resolve #1832? I have the same problem with import/named in monorepo structure. It doesn't work for yarn workspaces.

paztis · 2020-07-31T10:01:27Z

No, I think. It only verify the import path is correctly written. Not it the content of the import is good.
We pack can do it potentially. Or typescript

SanjiKir · 2020-07-31T10:07:58Z

@paztis Yeah we are migrating to TS so far, so it will take some time since the project is huge. I didn't get the last sentence of yours
We pack can do it potentially. What do you mean by that?

paztis · 2020-07-31T11:58:56Z

Sorry autocorrect from my phone. We can do it with webpack or typescript I said. In webpack there's an option to check the name import really exist in the import file Le ven. 31 juil. 2020 à 12:08, Shamanin Kirill <notifications@github.com> a écrit :

…

@paztis <https://github.com/paztis> Yeah we are migrating to TS so far, so it will take some time since the project is huge. I didn't get the last sentence of yours We pack can do it potentially. What do you mean by that? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1696 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABIKMMNC26KBLPJHF4BTFJLR6KJZBANCNFSM4LSVAZOA> .

paztis · 2020-08-05T12:44:09Z

@ljharb what is the review status now ?

caseyWebb · 2020-08-25T06:33:27Z

@paztis I found your fork on npm, but how can I actually use it?

I added "plugins": ["@paztis/eslint-plugin-import"] to my .eslintrc, but I can't figure out how to configure rules. e.g. "import/order", "@pastiz/eslint-plugin-import/order", and half a dozen other combinations all throw "Definition for rule was not found" errors.

ljharb · 2020-08-25T06:47:03Z

@paztis would you mind rebasing this on top of latest master (which will remove all merge commits) and ideally condense the commits? There's 34 right now.

I can handle #1696 (comment) for you after that's done, and give it a fresh review.

paztis · 2020-08-25T12:36:40Z

In fact the problem is Airbnb is pointing to original eslint-plugin-import, which conflict with mine . That's why I push to merge my changes. To use it, the only way I found is to use symlinks in my local environment: I symlink original one to mine. Le mar. 25 août 2020 à 08:47, Jordan Harband <notifications@github.com> a écrit :

…

@paztis <https://github.com/paztis> would you mind rebasing this on top of latest master (which will remove all merge commits) and ideally condense the commits? There's 34 right now. I can handle #1696 (comment) <#1696 (comment)> for you after that's done, and give it a fresh review. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1696 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABIKMMJCMELSI66K27ETSO3SCNM7NANCNFSM4LSVAZOA> .

paztis · 2020-08-27T23:18:56Z

@ljharb rebase on top of master is done now. I've verify the final content is exactly the same as on my old branch.
You can do the merge ?

michaelfarrell76 · 2020-09-03T18:51:36Z

so excited for this! thank you all so much 🙏

paztis · 2020-09-03T19:11:15Z

Me too ^^
@ljharb any news for the merge ?

ljharb · 2020-09-05T21:18:27Z

@paztis thanks! I've rebased this PR.

I've split out the isExternalModule bugfix into its own commit. That commit includes a test for order that fails without the fix, but i can't come up with a test for extensions that fails without the fix. Got any ideas?

Also, the second commit is quite large, so it's hard to me to understand everything it's doing. Can any parts of it be split into separate commits, that could land "first" - before the primary fix in the PR?

paztis · 2020-09-05T21:57:42Z

For the extension, you were testing only the name not the real path. If an alias exist for this name, potentially it is not an external. Is package value was potentially incorrect, then isUseOfExtensionRequired(extension, package) Will fail I just fix the usage of is external module because the last parameters was missing. For the second commit, tell me which point you want to check. Le sam. 5 sept. 2020 à 23:18, Jordan Harband <notifications@github.com> a écrit :

…

@paztis <https://github.com/paztis> thanks! I've rebased this PR. I've split out the isExternalModule bugfix into its own commit. That commit includes a test for order that fails without the fix, but i can't come up with a test for extensions that fails without the fix. Got any ideas? Also, the second commit is quite large, so it's hard to me to understand everything it's doing. Can any parts of it be split into separate commits, that could land "first" - before the primary fix in the PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1696 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABIKMMPAHEG7762HLSNGV3DSEKTLBANCNFSM4LSVAZOA> .

paztis · 2020-09-17T09:50:22Z

@ljharb, any news ?

buchanaf · 2020-10-20T18:54:10Z

Hey guys (@ljharb and @paztis ), really looking forward to having this support. Is there anything I could do to help speed this along so this PR doesn't get lost in purgatory?

paztis · 2020-10-20T18:58:11Z

I don't know in fact. I'm still waiting the approval for the merge.

michaelfarrell76 · 2021-02-04T06:56:55Z

thanks for merging this! @ljharb any idea when a new version will be published to npm?

JEROMEH added 2 commits March 24, 2020 09:03

[Fix] no-cycle/extensions: fix isExternalModule usage

e22fc53

paztis mentioned this pull request Mar 24, 2020

[import/order] 2.20.1 Windows only: Resolver cannot distinguish external and internal imports #1655

Closed

ljharb reviewed Mar 24, 2020

View reviewed changes

This was referenced Apr 9, 2020

import/no-extraneous-dependencies globs doesn't work on deeper folder tree (monorepo) #1302

Closed

[import/no-extraneous-dependencies]: better support for monorepos #1650

Closed

Enhancement: Monorepos and no-extraneous-dependencies #1174

Closed

ljharb requested changes Jun 5, 2020

View reviewed changes

ljharb approved these changes Jan 23, 2021

View reviewed changes

ljharb merged commit 802ce7d into import-js:master Jan 25, 2021

This was referenced May 14, 2021

import/no-extraneous-dependencies and import/order false postives since version 2.23 #2065

Closed

"peerDependencies" option has no effect in "import/no-extraneous-dependencies" rule since 2.23.0 #2058

Closed

kevin940726 mentioned this pull request Jun 23, 2021

Fix eslint-import resolver with extraneous dependencies WordPress/gutenberg#32906

Merged

7 tasks

fisker mentioned this pull request Jul 21, 2021

Remove require-json-extensions rule prettier/prettier#11233

Merged

4 tasks

Correct module real path resolution #1696

Correct module real path resolution #1696

Conversation

paztis commented Mar 24, 2020

coveralls commented Mar 24, 2020

coveralls commented Mar 24, 2020

coveralls commented Mar 24, 2020

coveralls commented Mar 24, 2020 • edited

paztis commented Mar 24, 2020

ljharb Mar 24, 2020

Choose a reason for hiding this comment

paztis Mar 25, 2020

Choose a reason for hiding this comment

ljharb Jun 5, 2020

Choose a reason for hiding this comment

paztis Jul 29, 2020

Choose a reason for hiding this comment

paztis commented Mar 25, 2020

paztis commented Mar 28, 2020

paztis commented Apr 2, 2020

paztis commented Apr 8, 2020

jimisaacs commented Apr 14, 2020

geoffp commented Apr 15, 2020

ljharb left a comment

Choose a reason for hiding this comment

ljharb Jun 5, 2020

Choose a reason for hiding this comment

paztis commented Jul 29, 2020

SanjiKir commented Jul 31, 2020

paztis commented Jul 31, 2020

SanjiKir commented Jul 31, 2020

paztis commented Jul 31, 2020 via email

paztis commented Aug 5, 2020

caseyWebb commented Aug 25, 2020

ljharb commented Aug 25, 2020

paztis commented Aug 25, 2020 via email

paztis commented Aug 27, 2020 • edited

michaelfarrell76 commented Sep 3, 2020

paztis commented Sep 3, 2020

ljharb commented Sep 5, 2020

paztis commented Sep 5, 2020 via email

paztis commented Sep 17, 2020

buchanaf commented Oct 20, 2020

paztis commented Oct 20, 2020

michaelfarrell76 commented Feb 4, 2021

coveralls commented Mar 24, 2020 •

edited

paztis commented Aug 27, 2020 •

edited