Allow core modules to resolve files inside declared modules #891
Conversation
src/core/importType.js
Outdated
| @@ -13,8 +13,9 @@ export function isAbsolute(name) { | |||
| } | |||
|
|
|||
| export function isBuiltIn(name, settings) { | |||
| const baseModule = name.split('/')[0] | |||
There was a problem hiding this comment.
what if the core module name is @foo/bar?
There was a problem hiding this comment.
That's a good point, thanks. Maybe we need something better than a simple split[0]. Let me try.
There was a problem hiding this comment.
How would you feel about replacing the base module name logic with the following?
function baseModule(name) {
const parts = name.split('/')
if (name.startsWith('@')) { return parts.slice(0, 2).join('/') }
return parts[0];
}
baseModule('foo') //=> "foo"
baseModule('foo/bar/baz') //=> "foo"
baseModule('@bat/foo/bar/baz') //=> "@bat/foo"There was a problem hiding this comment.
This logic already exists in the project: https://github.com/benmosher/eslint-plugin-import/blob/ee5a986cfd3ed9d97857889655a969031f65f66d/src/core/importType.js#L31-L33 (and it's best to avoid directly indexing into arrays)
Something like this could be good:
function baseModule(name) {
if (isScoped(name)) {
const [scope, pkg] = name.split('/');
return `${scope}/${pkg}`;
}
const [pkg] = name.split('/');
return pkg;
}There was a problem hiding this comment.
Thanks for pointing that out. I'll see if I can integrate that.
|
Just integrated your example logic, along with some tests for scoped packages. |
|
|
||
| const electronContext = testContext({ 'import/core-modules': ['electron'] }) | ||
| expect(importType('electron', electronContext)).to.equal('builtin') | ||
|
|
||
| const scopedContext = testContext({ 'import/core-modules': ['@org/foobar'] }) |
There was a problem hiding this comment.
we should probably also support a value of @org here - that way you can mark an entire package scope as builtin
There was a problem hiding this comment.
I'm not sure if that's needed. I work at an org that uses private packages a lot, and I'd fully expect to have to include each imported package one by one. It seems like it should be a bug to accidentally import @myorg and whitelist all @myorg/* packages. What do you think?
There was a problem hiding this comment.
I think that it'd be a fine addition later, so there's no need to block this PR on it :-)
|
I'm happy with this PR if you are! But it looks like the Appveyor build is failing even though all tests are passing. It looks like this error has happened earlier in the project history too, so I don't think it's part of this PR. Anything else I can help with before we merge this? |
|
Don't worry about the appveyor build for now. Let's get some reviews from the other collabs. |
|
@jfmengels you're off the hook unless you'd like to comment. I'll merge when I can, with changelog notes. thanks @mplewis! |
|
Hey @benmosher! I was wondering if we could merge this into eslint-plugin-import before it gets stale against master. Thanks. |
|
Makes sense, would merge it now but I'm on my phone and the Appveyor noise is blocking me 😓 |
Fixes https://travis-ci.org/avajs/ava/jobs/291603944 by changing the name of the module being imported. This failure is most likely caused by <import-js/eslint-plugin-import#891>. I've updated the package lock to pull in the newer eslint-plugin-import as well.
Fixes https://travis-ci.org/avajs/ava/jobs/291603944 by changing the name of the module being imported. This failure is most likely caused by <import-js/eslint-plugin-import#891>. I've updated the package lock to pull in the newer eslint-plugin-import as well.
Closes #886.
Lets users add packages to
core-modulessuch aselectron, then import resources from paths such aselectron/some/path/to/resource.jsonwithout encountering lint errors.You have edit access to this branch – let me know if there is anything else you're looking for. Thanks!