closes #1293. allows aliases that start with @ to be caught as internal #1294
Conversation
jeffshaver
force-pushed
the
master
branch
3 times, most recently
from
91a439c
to
3343898
Compare
4 similar comments
src/core/importType.js
Outdated
| function baseModule(name) { | ||
| if (isScoped(name)) { | ||
| function baseModule(name, path) { | ||
| if (isScoped(name, path)) { |
There was a problem hiding this comment.
i don't think it makes sense to make isScoped check isExternalPath; i think all the places currently calling isScoped should be explicitly handling isExternalPath first.
There was a problem hiding this comment.
I can do that. It does seem that "scoped" packages have a connotation meaning "external". So I thought it would be fine putting it in isScoped because scoped modules have to be external. Will update shortly
There was a problem hiding this comment.
Actually... I don't even know how to solve this? Move the isScoped check down underneath isInternalModulein the lodash.cond call? The problem I am seeing is that isScoped means external, but isExternalModule doesn't check it. It is checked by the lodash.cond call
There was a problem hiding this comment.
Seems like moving the internal check above scoped and external solved that problem and let me revert most of the other changes
There was a problem hiding this comment.
Scoped packages are the same as anything in node_modules; they're things installed from npm. I don't think "scoped" means "external".
|
@ljharb updated per your comments. Appreciate you looking at this! |
|
@ljharb anything I can do to help unbreak master? |
|
@jeffshaver open another PR that fixes the tests? :-D |
|
@ljharb. I'll take a look later today to see if I can fix some |
|
@ljharb since my other pr got merged (thanks! sorry we couldn't fix that one test), i rebased my branch with upstream master. just wanted to let you know |
|
@jeffshaver oh we fixed it! thanks for the PR, and the ping. |
| [isExternalModule, constant('external')], | ||
| [isScoped, constant('external')], | ||
| [isInternalModule, constant('internal')], |
There was a problem hiding this comment.
This looks good, but there is the possibility this will be a breaking change - however, since it doesn't break any tests, and since it's unlikely anyone is depending on the inability to mark an external module as "internal", it's probably fine.
|
@ljharb appreciate it! When do you think a new version will be released with these changes? |
|
Probably not super quickly; ping me again in a week if it’s not done by then. |
|
Have the PR been released? My '@' alias imports are still 'unknown' type with the latest eslint-plugin-import. |
@ljharb as we talked about in the issue, here is the fix. Please let me know if there are any issues. Or if you think we need more tests for this case. I added one. There were already a ton that didn't pass and didn't think I should fix them in this PR.
Fixes #1293. Unblocked by #1295.