import/extensions should have a ignorePackages option. Fixes #414 #827

collinsauve · 2017-05-10T16:58:53Z

For issue #414

Unfortunately I cannot run the tests from this repo on my machine (lots of unexplained failures) so I will have trouble writing additional tests for this new behavior.

…s#414

coveralls · 2017-05-10T17:28:18Z

Coverage increased (+0.01%) to 94.927% when pulling 5912a70 on collinsauve:issue414 into bd0e5e3 on benmosher:master.

coveralls · 2017-05-10T17:34:57Z

Coverage increased (+0.01%) to 94.927% when pulling 5912a70 on collinsauve:issue414 into bd0e5e3 on benmosher:master.

ljharb

I think rather than expanding the string option, it would be better to add an object option.

I would want ignorePackages for all enforcement, always or never, and arguably that should be the default (assuming it only applies to the "main", and never to any additional entry points).

ljharb · 2017-05-10T18:31:01Z

docs/rules/extensions.md

@@ -86,6 +86,30 @@ import express from 'express/index.js';
 import * as path from 'path';
 ```

+The following patterns are considered problems when configuration set to "ignorePackages":


let's add an example of require('foo/bar.js') still being required with both "always" and "ignorePackages" set.

this still needs an example, and a matching test

ljharb · 2017-05-10T18:31:40Z

src/rules/extensions.js

      if (!has(modifiers, extension)) { modifiers[extension] = defaultConfig }
-      return modifiers[extension] === 'always'
+      const modifier = modifiers[extension]
+      return (!isModule && modifier === 'ignorePackages') || modifier === 'always'


it should not be ignored just because it's an external package, rather only when it's the "main" of an external package.

In other words, require('foo') and require('@foo/bar') should be ignored, but subpaths under these should not.

Good catch! I'm not well versed in all of the possible import permutations, but here's a regex I just made up. Can you tell me if I'm missing something?

^(([\w]((?!\/).)*)|(@[^\/]+\/?[^\/]+))$

This matches the following:

foo foo2 foo.js @foo @foo2 @foo.js @foo/bar @foo2/bar @foo2.js/bar @foo2.js/bar.js

And will not match the following:

foo/bar foo2/bar foo/bar2 foo/bar.js foo/bar/hello @foo2/bar/hello @foo2/bar/hello.js

I'd really prefer to avoid complex regexes if we can, but your matches look good.

This regex is probably better understood if I split it:

// src/core/importType.js // Similar to isExternalModule, but does not allow "/" const externalModuleMainRegExp = /^[\w]((?!\/).)*$/ export function isExternalModuleMain(path, name, settings) { return externalModuleMainRegExp.test(name) && isExternalPath(path, name, settings) } // Similar to isScoped, but only allows 1 occurrence of "/" const scopedMainRegExp = /^@[^\/]+\/?[^\/]+$/ export function isScopedMain(name) { return scopedMainRegExp.test(name) }

Then just OR these.

collinsauve · 2017-05-10T20:14:19Z

@ljharb object options are already taken by the extensions. Adding anything to the object options will create ambiguity.

ljharb · 2017-05-10T20:17:21Z

That's true. We could allow for a separate object, though, in either order, and later, migrate the extensions options to an "extensions" key in that new object, and deprecate/remove the top-level extensions config object.

collinsauve · 2017-05-10T20:40:46Z

Say we have a new object type (for now called newProperties ) with props like extensions, and ignorePackages. We now need to add a new item to meta.schema.anyOf.

As far as I understand this schema system, since patternProperties catches all objects we can't use args [newProperties]. For the same reason we also cant use [string, newProperties]. We need to use [string, patternProperties, newProperties] or [patternProperties, newProperties].

Then what's the migration path? Perhaps deprecate [patternProperties] and [string, patternProperties] in verion N and add support for [newProperties] and [string, newProperties] in version N+1? ... And then remove support for [string, patternProperties, newProperties] and [patternProperties, newProperties] in version N + 2.

ljharb · 2017-05-10T21:37:10Z

ok, so the current schema allows any of:

one item, "always" or "never"
one item, an object of "pattern properties"
two items, "always/never" and an object of "pattern properties".

Indeed, this is problematic, and "patternProperties" should never have had top-level configurable keys.

It seems like the migration path is the following schema:

one item, "always" or "never"
new two items, "always/never" and an object with a "pattern" key (containing pattern properties)
new two items, "always/never" and an object with an "ignorePackages" key
new two items, "always/never" and an object with an "ignorePackages" key and a "pattern" key
new one item, an object with an "ignorePackages" key
new one item, an object with an "ignorePackages" key and a "pattern" key
one item, an object of "pattern properties"
two items, "always/never" and an object of "pattern properties".

And then, we'd deprecate and eventually remove the "object of pattern properties" schemas (the last two above), and when they were removed, we'd be able to collapse the 5 new schema types into one or two.

collinsauve · 2017-05-11T19:21:00Z

Thanks for that @ljharb, makes sense now. I will try to implement your suggestions tonight or tomorrow.

coveralls · 2017-05-12T21:50:44Z

Coverage decreased (-0.2%) to 94.743% when pulling 70ea249 on collinsauve:issue414 into bd0e5e3 on benmosher:master.

collinsauve · 2017-05-18T21:05:31Z

I can't really work on building tests for this this (to fix coverage issue) due to #838

millerized · 2017-06-30T15:45:18Z

Hi, @collinsauve @ljharb – great work here. I am not a formal contributor yet, but I am curious if there is anything I can do help get this moving again?

🍻

ljharb · 2017-06-30T16:19:50Z

@millerized if you can contribute a commit to improve coverage on top of this PR, and link it here, I can push it to the branch - I think that's the main thing that's missing.

jdalton · 2017-07-13T17:05:30Z

Awesome PR! Was just looking for a way to configure this.

millerized · 2017-10-31T13:15:05Z

Hi, @ljharb @collinsauve – I’ve rebased this branch with upstream to fix conflicts and added some test coverage around the ignorePackages option. Please see millerized#1. Please let me know if there is still missing coverage and I will get it in asap. 🍻

ljharb · 2017-10-31T14:14:47Z

Thanks! I've updated the PR with those commits, and will review later today.

coveralls · 2017-10-31T16:26:52Z

Coverage increased (+0.05%) to 96.084% when pulling a8e8bfb on collinsauve:issue414 into a5844d5 on benmosher:master.

millerized · 2017-10-31T19:53:24Z

Thanks! I've updated the PR with those commits, and will review later today.

Thanks, @ljharb!

ljharb

LGTM pending one more comment!

ljharb · 2017-10-31T20:12:12Z

docs/rules/extensions.md

@@ -86,6 +86,30 @@ import express from 'express/index.js';
 import * as path from 'path';
 ```

+The following patterns are considered problems when configuration set to "ignorePackages":


this still needs an example, and a matching test

ref: https://github.com/benmosher/eslint-plugin-import/pull/827/files/a8e8bfb664c560665fcdc92113c1b927c9981889#r115817771

millerized · 2017-11-01T01:42:52Z

this still needs an example, and a matching test

Thanks for the reminder, @ljharb. I beleive these commits should suffice. lmk what you think.

ljharb

Thanks, LGTM!

ljharb · 2017-11-01T02:50:46Z

I'll merge this tomorrow if there's no objections.

coveralls · 2017-11-01T03:04:58Z

Coverage decreased (-0.01%) to 96.082% when pulling 81d3b36 on collinsauve:issue414 into eca7b4d on benmosher:master.

millerized · 2017-11-01T13:27:42Z

continuous-integration/appveyor/pr — AppVeyor build failed

Unsure about the AV CI failure. It appears to be a false negative. Perhaps it needs a re-run, @ljharb.

Coverage decreased (-0.01%) to 96.082% when pulling 81d3b36 on collinsauve:issue414 into eca7b4d on benmosher:master.

Coverage decreasing seems odd.

ljharb · 2017-11-01T17:42:10Z

appveyor is passing, but coveralls is throwing errors; you can ignore it.

millerized · 2017-11-02T01:27:29Z

appveyor is passing, but coveralls is throwing errors; you can ignore it.

Gtk. Thanks, @ljharb. Any last items to address or are we ready to merge?

import/extensions should have a ignorePackages option. Fixes import-j…

8def087

…s#414

collinsauve mentioned this pull request May 10, 2017

import/extensions should have a ignorePackages option #414

Closed

collinsauve force-pushed the issue414 branch from c7f0e0d to 5912a70 Compare May 10, 2017 17:08

ljharb requested changes May 10, 2017

View reviewed changes

collinsauve force-pushed the issue414 branch 2 times, most recently from b487d2c to 70ea249 Compare May 12, 2017 21:44

Fix lint no-useless-escape

62ff6e5

millerized mentioned this pull request Oct 30, 2017

import/extensions should have a ignorePackages option. Fixes #414 [@millerized] millerized/eslint-plugin-import#1

Closed

Add some test coverage for ignorePackages exts

fef0ab0

ljharb force-pushed the issue414 branch from 70ea249 to 00062fb Compare October 31, 2017 14:13

ljharb force-pushed the issue414 branch from 00062fb to a8e8bfb Compare October 31, 2017 14:17

ljharb reviewed Oct 31, 2017

View reviewed changes

ljharb added accepted enhancement labels Oct 31, 2017

ljharb added the semver-minor label Oct 31, 2017

ljharb self-assigned this Oct 31, 2017

millerized added 3 commits October 31, 2017 16:35

Should cover cherry-picked package methods

2fc4577

ref: https://github.com/benmosher/eslint-plugin-import/pull/827/files/a8e8bfb664c560665fcdc92113c1b927c9981889#r115817771

Document cherry-picked package method

7316dda

ref: https://github.com/benmosher/eslint-plugin-import/pull/827/files/a8e8bfb664c560665fcdc92113c1b927c9981889#r115817771

Typos

81d3b36

ljharb force-pushed the issue414 branch from a8e8bfb to 81d3b36 Compare November 1, 2017 02:49

ljharb approved these changes Nov 1, 2017

View reviewed changes

ljharb requested review from benmosher and jfmengels November 1, 2017 02:50

ljharb merged commit ee51581 into import-js:master Nov 2, 2017

collinsauve deleted the issue414 branch November 2, 2017 03:32

michaelhogg mentioned this pull request Feb 5, 2018

Cut new version #976

Closed

renovate bot mentioned this pull request Feb 22, 2018

Update dependency eslint-plugin-import to v2.11.0 Urigo/tortilla#68

Merged

renovate bot mentioned this pull request Mar 22, 2018

Update eslint-plugin-import to 2.12.0 sbardian/DebtTrackerAPI#25

Merged

ljharb mentioned this pull request Apr 20, 2018

"import/extensions": ignorePackages value does not inforce the use of extensions. #1083

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

import/extensions should have a ignorePackages option. Fixes #414 #827

import/extensions should have a ignorePackages option. Fixes #414 #827

collinsauve commented May 10, 2017 •

edited

Loading

coveralls commented May 10, 2017 •

edited

Loading

coveralls commented May 10, 2017 •

edited

Loading

ljharb left a comment

ljharb May 10, 2017

ljharb Oct 31, 2017

ljharb May 10, 2017

collinsauve May 10, 2017

ljharb May 10, 2017

collinsauve May 11, 2017 •

edited

Loading

collinsauve commented May 10, 2017

ljharb commented May 10, 2017

collinsauve commented May 10, 2017 •

edited

Loading

ljharb commented May 10, 2017

collinsauve commented May 11, 2017

coveralls commented May 12, 2017 •

edited

Loading

collinsauve commented May 18, 2017

millerized commented Jun 30, 2017

ljharb commented Jun 30, 2017

jdalton commented Jul 13, 2017

millerized commented Oct 31, 2017 •

edited

Loading

ljharb commented Oct 31, 2017

coveralls commented Oct 31, 2017 •

edited

Loading

millerized commented Oct 31, 2017

ljharb left a comment

ljharb Oct 31, 2017

millerized commented Nov 1, 2017

ljharb left a comment

ljharb commented Nov 1, 2017

coveralls commented Nov 1, 2017 •

edited

Loading

millerized commented Nov 1, 2017

ljharb commented Nov 1, 2017

millerized commented Nov 2, 2017

import/extensions should have a ignorePackages option. Fixes #414 #827

import/extensions should have a ignorePackages option. Fixes #414 #827

Conversation

collinsauve commented May 10, 2017 • edited Loading

coveralls commented May 10, 2017 • edited Loading

coveralls commented May 10, 2017 • edited Loading

ljharb left a comment

Choose a reason for hiding this comment

ljharb May 10, 2017

Choose a reason for hiding this comment

ljharb Oct 31, 2017

Choose a reason for hiding this comment

ljharb May 10, 2017

Choose a reason for hiding this comment

collinsauve May 10, 2017

Choose a reason for hiding this comment

ljharb May 10, 2017

Choose a reason for hiding this comment

collinsauve May 11, 2017 • edited Loading

Choose a reason for hiding this comment

collinsauve commented May 10, 2017

ljharb commented May 10, 2017

collinsauve commented May 10, 2017 • edited Loading

ljharb commented May 10, 2017

collinsauve commented May 11, 2017

coveralls commented May 12, 2017 • edited Loading

collinsauve commented May 18, 2017

millerized commented Jun 30, 2017

ljharb commented Jun 30, 2017

jdalton commented Jul 13, 2017

millerized commented Oct 31, 2017 • edited Loading

ljharb commented Oct 31, 2017

coveralls commented Oct 31, 2017 • edited Loading

millerized commented Oct 31, 2017

ljharb left a comment

Choose a reason for hiding this comment

ljharb Oct 31, 2017

Choose a reason for hiding this comment

millerized commented Nov 1, 2017

ljharb left a comment

Choose a reason for hiding this comment

ljharb commented Nov 1, 2017

coveralls commented Nov 1, 2017 • edited Loading

millerized commented Nov 1, 2017

ljharb commented Nov 1, 2017

millerized commented Nov 2, 2017

collinsauve commented May 10, 2017 •

edited

Loading

coveralls commented May 10, 2017 •

edited

Loading

coveralls commented May 10, 2017 •

edited

Loading

collinsauve May 11, 2017 •

edited

Loading

collinsauve commented May 10, 2017 •

edited

Loading

coveralls commented May 12, 2017 •

edited

Loading

millerized commented Oct 31, 2017 •

edited

Loading

coveralls commented Oct 31, 2017 •

edited

Loading

coveralls commented Nov 1, 2017 •

edited

Loading