[Fix] no-extraneous-dependencies: add ESM intermediate package.json support

[paztis]

No description provided.

[coveralls]

Coverage increased (+2.3%) to 82.607% when pulling ff2bcf0 on paztis:master into 4079482 on benmosher:master.

[coveralls]

Coverage increased (+2.3%) to 82.607% when pulling ff2bcf0 on paztis:master into 4079482 on benmosher:master.

[coveralls]

Coverage increased (+2.3%) to 82.621% when pulling 9552589 on paztis:master into 4079482 on benmosher:master.

[ljharb]

fix looks good; now we just need a regression test :-)

[paztis]

I don't know how to enter in this case, so don't know what to add on unit tests... Le mar. 8 juin 2021 à 16:50, Jordan Harband ***@***.***> a écrit :

…

fix looks good; now we just need a regression test :-) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2121 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABIKMMJOXYLUS4IKBNKBDTLTRYU5HANCNFSM46JC6D3A> .

[ljharb]

Hopefully the issue OP can provide repro details

[paztis]

correct fix with test case added now, as described by @ertrzyiks in #2120

[ertrzyiks]

Works like a charm in my case 👍

[ljharb]

@paztis thanks! I've rebased this (please reset your local master to the remote one before making further changes, so the rebase isn't lost), but the tests don't fail when the changes are backed out, so they're not actually testing the changes.

[paztis]

You wait an action from me to do the merge ? Or it is ok ?

[benjamingr]

@paztis what Jordan is saying is that the PR is good but the test isn't since if he reverts those changes (but keeps the test) the test passes.

[JCB-K]

Hi! 👋 . I adjusted the tests on this branch so that they're true regression tests. I made a PR against @paztis 's fork, so if you merge that PR into paztis:master, and then merge this PR into benmosher:master (after review), it should be all good. not sure if that's the easiest way to handle this?

[ljharb]

I’ll check later today; if the tests fail without the fix, and pass with it, we’re good to go :-)

[paztis]

I've added a valid test that is now passing without the fix.
The previous one was only passing thanks to the getModuleOriginalName() case that was finding the correct name first.
Nw with the webpack alias added, the getModuleOriginalName() found the name "alias" and let the real moduleName resolution do the job correctly.

[paztis]

Hi! 👋 . I adjusted the tests on this branch so that they're true regression tests. I made a PR against @paztis 's fork, so if you merge that PR into paztis:master, and then merge this PR into benmosher:master (after review), it should be all good. not sure if that's the easiest way to handle this?

Sorry just see you message after I rebate from upstream to avoir rebate unit test failure...

So I delete and recommit all my job.
You can look at my current unit test and reel me if it is enough. You own can be better perhaps

[ljharb]

@paztis unfortunately the tests now seem to pass even without the fix. We can't merge this until there's tests that fail without the fix.

[paztis]

Really ? They were failing with a null pointer on my side...

[paztis]

Ok again update the unit tests (and simplify also the process)
If you take my new unit tests without my code fixes, you must have this exception

AssertionError [ERR_ASSERTION] [ERR_ASSERTION]: Should have no errors but had 1: [

{
ruleId: 'no-extraneous-dependencies',
severity: 1,
message: "'alias' should be listed in the project's dependencies. Run 'npm i -S alias' to add it",
line: 1,
column: 1,
nodeType: 'ImportDeclaration',
endLine: 1,
endColumn: 39
}
]

I again face rebase problematics... sorry for that ^^

[JCB-K]

Shouldn't the test fail with TypeError: Cannot read property 'split' of undefined if the fix isn't applied? Since that's the bug that this PR addresses.

[paztis]

Yes it is supposed

[JCB-K]

@paztis the test I made does just that:

› yarn test
  2100 passing (10s)
  1 failing

  1) no-extraneous-dependencies invalid import "esm-package-not-in-pkg-json/esm-module";:
     TypeError: Cannot read property 'split' of undefined
Occurred while linting /Users/jacob/Mentimeter/eslint-plugin-import/tests/files/foo.js:1
      at checkDependencyDeclaration (src/rules/no-extraneous-dependencies.js:133:40)
      at reportIfMissing (src/rules/no-extraneous-dependencies.js:187:38)
      at commonjs (src/rules/no-extraneous-dependencies.js:264:7)
      at checkSourceValue (utils/moduleVisitor.js:29:5)
      at checkSource (utils/moduleVisitor.js:34:5)
      at node_modules/eslint/lib/linter/safe-emitter.js:45:58
      at Array.forEach (<anonymous>)
      at Object.emit (node_modules/eslint/lib/linter/safe-emitter.js:45:38)
      at NodeEventGenerator.applySelector (node_modules/eslint/lib/linter/node-event-generator.js:293:26)
      at NodeEventGenerator.applySelectors (node_modules/eslint/lib/linter/node-event-generator.js:322:22)
      at NodeEventGenerator.enterNode (node_modules/eslint/lib/linter/node-event-generator.js:336:14)
      at CodePathAnalyzer.enterNode (node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:711:23)
      at node_modules/eslint/lib/linter/linter.js:956:32
      at Array.forEach (<anonymous>)
      at runRules (node_modules/eslint/lib/linter/linter.js:951:15)
      at Linter._verifyWithoutProcessors (node_modules/eslint/lib/linter/linter.js:1177:31)
      at Linter.verify (node_modules/eslint/lib/linter/linter.js:1241:21)
      at runRuleForItem (node_modules/eslint/lib/rule-tester/rule-tester.js:560:37)
      at testInvalidTemplate (node_modules/eslint/lib/rule-tester/rule-tester.js:658:28)
      at Context.<anonymous> (node_modules/eslint/lib/rule-tester/rule-tester.js:897:25)
      at processImmediate (internal/timers.js:461:21)

I've separated the commits into 1 commit that adds the test, and 1 commit that contains your change: feel free to cherry-pick from this branch however you like. https://github.com/JCB-K/eslint-plugin-import/commits/fix-tests

[paztis]

@JCB-K the branch you show me didn't include the last changes, nor the last unit tests

[paztis]

there's 2 fixes in fact: one about the NPE in case of undefined name, and another about a recursion in the name search in case of intermediary package.json found in esm modules.

[JCB-K]

Indeed and both are still fixed on that branch, and its up-to-date with benmosher:master. Either way, feel free to take the tests as you see fit, either by merging JCB-K:fix-tests into your branch, into benmosher:master, or by just copying over the tests manually. I'm just happy to help out on a bugfix here as it affects me too 🙂

[timleslie]

Hi @ljharb, I was wondering if there are plans to ship a new release which contains this fix? This bug currently bites us when we refresh our yarn.lock with ^ range semver, so it'd be nice to be able to get over that speed bump. We can manually work around this, so it's definitely not a blocker, but it'd be a nice to smooth out our maintenance processes. Thanks for you work on this package!