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

fix: aliased internal modules that look like core modules #1297

Merged
merged 2 commits into from
Apr 11, 2019

Conversation

echenley
Copy link
Contributor

@echenley echenley commented Mar 6, 2019

What

Update isBuiltIn to return false if a path is returned from a resolver.
Previously an alias like 'constants/foo' would be classified as a builtin rather than internal since 'constants' is a core module of node. Fixes one of the issues mentioned in #1034.

Update webpack resolver to attempt to resolve modules internally before classifying them as builtin.

Add several tests to cover these cases.

Example

project
└── src
    └── constants
        └── index.js
{
  "settings": {
    "import/resolver": {
      "node": {
        "paths": ["/project/src"]
      }
    }
  }
}

Result:

// BEFORE
import { FOO } from 'constants/index'
import { isObject } from 'lodash'

// AFTER
import { isObject } from 'lodash'
import { FOO } from 'constants/index'

tests/src/core/importType.js Outdated Show resolved Hide resolved
tests/src/core/importType.js Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Mar 6, 2019

Coverage Status

Coverage increased (+0.001%) to 97.863% when pulling ba0aed9 on echenley:ech/fix-isBuiltIn-local-aliases into 0ff1c83 on benmosher:master.

src/core/importType.js Outdated Show resolved Hide resolved
tests/src/core/importType.js Outdated Show resolved Hide resolved
tests/src/core/importType.js Outdated Show resolved Hide resolved
@ljharb ljharb requested a review from benmosher March 16, 2019 06:31
@karuana
Copy link

karuana commented Mar 22, 2019

When will this PR be merged?
I am waiting for this PR to be merged. 😭😭😭

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