Support scoped modules containing hyphens #744

rosswarren · 2017-02-13T11:04:22Z

I was having problems with the order rule and scoped modules. It was returning NaN for the rank. I discovered the regex was not matching our scope name which contained hyphens.

This regex should match the full range of scoped module names I think. Please check it though.

coveralls · 2017-02-13T11:18:27Z

Coverage increased (+0.003%) to 94.915% when pulling fc2569c on rosswarren:master into 0800951 on benmosher:master.

ljharb · 2017-02-13T18:54:19Z

src/core/importType.js

@@ -27,7 +27,7 @@ function isExternalModule(name, settings, path) {
  return externalModuleRegExp.test(name) && isExternalPath(path, name, settings)
 }

-const scopedRegExp = /^@\w+\/\w+/


would this be simpler as /^@[^\/]+\/[^\/]+?

Yes, just tested this on our codebase and that works 👍 I have updated the pull request.

coveralls · 2017-02-14T10:22:31Z

Coverage increased (+0.003%) to 94.915% when pulling 2fb246d on rosswarren:master into 0800951 on benmosher:master.

coveralls · 2017-02-14T10:22:31Z

Coverage increased (+0.003%) to 94.915% when pulling 2fb246d on rosswarren:master into 0800951 on benmosher:master.

coveralls · 2017-02-14T10:22:31Z

Coverage increased (+0.003%) to 94.915% when pulling 2fb246d on rosswarren:master into 0800951 on benmosher:master.

ljharb · 2017-02-14T16:38:23Z

Instead of regexes, should we use the official https://github.com/npm/validate-npm-package-name ? It'll be a scoped package if it's an invalid old one but a valid new one, and we won't have to maintain our own logic.

rosswarren · 2017-02-14T16:54:06Z

@ljharb I am not sure if that will help us with detection of scoped modules 🤔 . It will validate but not tell us what type it is.

ljharb · 2017-02-14T16:58:45Z

It returns two booleans, "old" and "new" - new meaning, including scoped. If they're both true it's scoped, if just "old" is true it's unscoped, if both are false, it's invalid.

rosswarren · 2017-02-14T21:31:25Z

@ljharb unfortunately this isn't how it works

ljharb · 2017-02-14T22:11:51Z

@rosswarren what about checking "new", and if it starts with an "@"?

That's an official npm package, and it'd really be nice to use that instead of messing around with regexps.

rosswarren · 2017-02-14T22:29:05Z

@ljharb OK, good idea. I will get it done!

rosswarren · 2017-02-15T10:23:04Z

@ljharb hmm, I tried to implement this but it seems that the isScoped function is passed the whole import for example @scope/donthaveit/lib/foo rather than just the package which would be @scope/donthaveit so you would still need to try and parse out the package name with some code or a regex.

rosswarren · 2017-02-15T19:00:01Z

@ljharb would it be a problem to only check if the first character is an "@"?. From NPM docs "If a package's name begins with @, then it is a scoped package". Not sure we really need to check anything else.

ljharb · 2017-02-15T19:45:01Z

@rosswarren Probably - but @foo isn't a scoped package, and could be a webpack alias - same with @.

rosswarren · 2017-02-15T20:14:26Z

@ljharb do you have any further suggestion given my previous comment? Not sure how to proceed here.

jfmengels

I'd be fine with sticking to a regex in this case. If someone can make it work using validate-npm-package-name, even better. But as @rosswarren said, we pass the whole path (which can be like @foo/bar/folder/file.js), which the package doesn't validate, and you'd pretty much need a regex anyway to extract the data.

jfmengels · 2017-02-16T21:26:37Z

tests/src/core/importType.js

@@ -31,6 +31,7 @@ describe('importType(name)', function () {
  it("should return 'external' for scopes packages", function() {
    expect(importType('@cycle/core', context)).to.equal('external')
    expect(importType('@cycle/dom', context)).to.equal('external')
+    expect(importType('@some-thing/something', context)).to.equal('external')


Could you add additional tests for the following cases? (and more if you think of some, better safe than sorry)

@something/some-thing

@something/something/something

have added 👍

coveralls · 2017-02-17T10:31:30Z

Coverage increased (+0.009%) to 94.924% when pulling 1fe3b4a on rosswarren:master into b5a962f on benmosher:master.

coveralls · 2017-02-17T10:31:30Z

Coverage increased (+0.009%) to 94.924% when pulling 1fe3b4a on rosswarren:master into b5a962f on benmosher:master.

coveralls · 2017-03-04T16:48:28Z

Coverage increased (+0.009%) to 94.924% when pulling bd243b2 on rosswarren:master into 98e7048 on benmosher:master.

ljharb · 2017-03-04T17:45:09Z

You could use fs methods to split up the path, without using a regex, but that's probably fine to leave for a future PR.

@rosswarren would you mind rebasing, so as to remove all merge commits?

coveralls · 2017-03-04T18:04:39Z

Coverage increased (+0.009%) to 94.924% when pulling 4d03274 on rosswarren:master into 98e7048 on benmosher:master.

rosswarren · 2017-03-06T22:51:25Z

@ljharb yeah that would require some more thinking, there is probably a better solution for this. However, right now of our code importing scoped modules is full of extra line breaks so I am anxious to see this fixed 😄

And yeah no problem I rebased the branch 👍

afairb · 2017-04-12T15:00:23Z

Hi. I see this PR is approved but not merged quite yet. Is there any chance it could get in soon as it's causing issues in my project when importing scoped modules? Thanks!

ljharb · 2017-04-12T16:32:10Z

The Windows build is still failing.

coveralls · 2017-06-05T11:05:43Z

Coverage increased (+0.008%) to 95.165% when pulling 0ad9659 on rosswarren:master into 3c46d30 on benmosher:master.

danny-andrews · 2017-07-25T15:51:33Z

Can we get this merged? I'm having this same issue. Is the Windows build still failing?

danny-andrews · 2017-07-25T16:05:20Z

You could use fs methods to split up the path, without using a regex, but that's probably fine to leave for a future PR.

Just out of curiosity, how would you do this? @ljharb

ljharb · 2017-07-25T16:31:13Z

@danny-andrews a combo of path.parse, splitting on path.delimiter, etc.

danny-andrews · 2017-07-25T16:51:57Z

Siiiiick. Thanks for merging!

danny-andrews · 2017-08-09T16:34:34Z

This looks like a good solution to this: https://github.com/mattdesl/require-package-name.

ljharb · 2017-08-09T17:55:25Z

It doesn't seem to work for relative paths tho.

danny-andrews · 2017-08-09T18:10:20Z

Brutal. Could use it in combination with https://www.npmjs.com/package/relative-require-regex?
Maybe not worth it for solving such a simple problem doe.

ljharb approved these changes Feb 13, 2017

View reviewed changes

rosswarren force-pushed the master branch from fc2569c to 2fb246d Compare February 14, 2017 10:08

jfmengels reviewed Feb 16, 2017

View reviewed changes

rosswarren force-pushed the master branch from 2fb246d to 1fe3b4a Compare February 17, 2017 10:11

rosswarren force-pushed the master branch from bd243b2 to 4d03274 Compare March 4, 2017 17:58

support scoped modules containing hyphens

0ad9659

rosswarren force-pushed the master branch from 4d03274 to 0ad9659 Compare June 5, 2017 10:59

ljharb requested review from benmosher and jfmengels June 5, 2017 16:12

danny-andrews mentioned this pull request Jul 25, 2017

import/order incorrectly classifies linked in modules. #882

Closed

ljharb merged commit 40f5635 into import-js:master Jul 25, 2017

benmosher added a commit that referenced this pull request Oct 18, 2017

#744 changelog note

8702af7

This was referenced Apr 15, 2018

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

Merged

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

Merged

Support scoped modules containing hyphens #744

Support scoped modules containing hyphens #744

Conversation

rosswarren commented Feb 13, 2017

coveralls commented Feb 13, 2017 • edited Loading

ljharb Feb 13, 2017

Choose a reason for hiding this comment

rosswarren Feb 14, 2017

Choose a reason for hiding this comment

coveralls commented Feb 14, 2017

coveralls commented Feb 14, 2017

coveralls commented Feb 14, 2017 • edited Loading

ljharb commented Feb 14, 2017

rosswarren commented Feb 14, 2017

ljharb commented Feb 14, 2017

rosswarren commented Feb 14, 2017

ljharb commented Feb 14, 2017

rosswarren commented Feb 14, 2017

rosswarren commented Feb 15, 2017

rosswarren commented Feb 15, 2017

ljharb commented Feb 15, 2017

rosswarren commented Feb 15, 2017

jfmengels left a comment

Choose a reason for hiding this comment

jfmengels Feb 16, 2017

Choose a reason for hiding this comment

rosswarren Feb 17, 2017

Choose a reason for hiding this comment

coveralls commented Feb 17, 2017

coveralls commented Feb 17, 2017 • edited Loading

coveralls commented Mar 4, 2017

ljharb commented Mar 4, 2017

coveralls commented Mar 4, 2017 • edited Loading

rosswarren commented Mar 6, 2017

afairb commented Apr 12, 2017

ljharb commented Apr 12, 2017

coveralls commented Jun 5, 2017 • edited Loading

danny-andrews commented Jul 25, 2017 • edited Loading

danny-andrews commented Jul 25, 2017 • edited Loading

ljharb commented Jul 25, 2017

danny-andrews commented Jul 25, 2017

danny-andrews commented Aug 9, 2017

ljharb commented Aug 9, 2017

danny-andrews commented Aug 9, 2017

coveralls commented Feb 13, 2017 •

edited

Loading

coveralls commented Feb 14, 2017 •

edited

Loading

coveralls commented Feb 17, 2017 •

edited

Loading

coveralls commented Mar 4, 2017 •

edited

Loading

coveralls commented Jun 5, 2017 •

edited

Loading

danny-andrews commented Jul 25, 2017 •

edited

Loading

danny-andrews commented Jul 25, 2017 •

edited

Loading