Support scoped modules containing hyphens - #744
Conversation
| return externalModuleRegExp.test(name) && isExternalPath(path, name, settings) | ||
| } | ||
|
|
||
| const scopedRegExp = /^@\w+\/\w+/ |
There was a problem hiding this comment.
would this be simpler as /^@[^\/]+\/[^\/]+?
There was a problem hiding this comment.
Yes, just tested this on our codebase and that works 👍 I have updated the pull request.
2 similar comments
|
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. |
|
@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. |
|
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. |
|
@ljharb unfortunately this isn't how it works
|
|
@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. |
|
@ljharb OK, good idea. I will get it done! |
|
@ljharb hmm, I tried to implement this but it seems that the |
|
@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. |
|
@rosswarren Probably - but |
|
@ljharb do you have any further suggestion given my previous comment? Not sure how to proceed here. |
jfmengels
left a comment
There was a problem hiding this comment.
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.
| 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') |
There was a problem hiding this comment.
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
1 similar comment
|
You could use @rosswarren would you mind rebasing, so as to remove all merge commits? |
|
@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 👍 |
|
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! |
|
The Windows build is still failing. |
|
Can we get this merged? I'm having this same issue. Is the Windows build still failing? |
Just out of curiosity, how would you do this? @ljharb |
|
@danny-andrews a combo of |
|
Siiiiick. Thanks for merging! |
|
This looks like a good solution to this: https://github.com/mattdesl/require-package-name. |
|
It doesn't seem to work for relative paths tho. |
|
Brutal. Could use it in combination with https://www.npmjs.com/package/relative-require-regex? |
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.