Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow core modules to resolve files inside declared modules #891

Merged
merged 4 commits into from
Aug 14, 2017

Conversation

mplewis
Copy link
Contributor

@mplewis mplewis commented Jul 9, 2017

Closes #886.

Lets users add packages to core-modules such as electron, then import resources from paths such as electron/some/path/to/resource.json without encountering lint errors.

You have edit access to this branch – let me know if there is anything else you're looking for. Thanks!

@@ -13,8 +13,9 @@ export function isAbsolute(name) {
}

export function isBuiltIn(name, settings) {
const baseModule = name.split('/')[0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if the core module name is @foo/bar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point, thanks. Maybe we need something better than a simple split[0]. Let me try.

Copy link
Contributor Author

@mplewis mplewis Jul 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out. I'll see if I can integrate that.

@coveralls
Copy link

coveralls commented Jul 9, 2017

Coverage Status

Coverage increased (+0.008%) to 95.967% when pulling 307ddeb on mplewis:master into c9dd91d on benmosher:master.

@coveralls
Copy link

coveralls commented Jul 9, 2017

Coverage Status

Coverage increased (+0.008%) to 95.967% when pulling 307ddeb on mplewis:master into c9dd91d on benmosher:master.

@mplewis
Copy link
Contributor Author

mplewis commented Jul 9, 2017

Just integrated your example logic, along with some tests for scoped packages.

@coveralls
Copy link

coveralls commented Jul 9, 2017

Coverage Status

Coverage increased (+0.03%) to 95.988% when pulling 2f2dfb0 on mplewis:master into c9dd91d on benmosher:master.

@coveralls
Copy link

coveralls commented Jul 9, 2017

Coverage Status

Coverage increased (+0.03%) to 95.988% when pulling 2f2dfb0 on mplewis:master into c9dd91d on benmosher:master.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!


const electronContext = testContext({ 'import/core-modules': ['electron'] })
expect(importType('electron', electronContext)).to.equal('builtin')

const scopedContext = testContext({ 'import/core-modules': ['@org/foobar'] })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably also support a value of @org here - that way you can mark an entire package scope as builtin

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it'd be a fine addition later, so there's no need to block this PR on it :-)

@mplewis
Copy link
Contributor Author

mplewis commented Jul 10, 2017

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?

@ljharb
Copy link
Member

ljharb commented Jul 11, 2017

Don't worry about the appveyor build for now. Let's get some reviews from the other collabs.

Copy link
Member

@benmosher benmosher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great, thanks! :shipit:

@benmosher
Copy link
Member

@jfmengels you're off the hook unless you'd like to comment. I'll merge when I can, with changelog notes. thanks @mplewis!

@mplewis
Copy link
Contributor Author

mplewis commented Aug 11, 2017

Hey @benmosher! I was wondering if we could merge this into eslint-plugin-import before it gets stale against master. Thanks.

@benmosher
Copy link
Member

Makes sense, would merge it now but I'm on my phone and the Appveyor noise is blocking me 😓

@ljharb ljharb merged commit cd77133 into import-js:master Aug 14, 2017
benmosher added a commit that referenced this pull request Oct 18, 2017
novemberborn added a commit to avajs/ava that referenced this pull request Oct 23, 2017
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.
novemberborn added a commit to avajs/ava that referenced this pull request Oct 24, 2017
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants