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 import type determination for monorepo setup w/ webpack resolver #1605

Merged
merged 2 commits into from Jan 14, 2020

Conversation

@skozin
Copy link
Contributor

skozin commented Jan 12, 2020

Fixes #1597.

This PR changes the logic of isExternalPath function so that it doesn't consider the import name. For a path to be external, it's now sufficient that it matches one of the items in the import/external-module-folders array specified in settings.

This is to support a setup where some of the modules are symlinked from node_modules into directories elsewhere in the project (called "workspaces" in Yarn terminology). For example, it's common to have a monorepo setup like this:

node_modules
  @my-scope
    core -> ../../packages/core
    utils -> ../../packages/utils
packages
  core
    package.json
    index.js
  utils
    package.json
    index.js
package.json

Usually, one wants these packages to be treated as external since they're normally installed from a registry. Intuitively, this should be achieved by adding packages to import/external-module-folders array. However, this didn't work in a project that uses the webpack resolver.

The reason is that webpack resolver returns a fully resolved path to the module's main file, e.g. for @my-scope/core it would be something like /home/me/project/packages/core/index.js. This path doesn't contain the package name, @my-scope/core, as a substring, so isExternalPath would consider this path as internal prior to the changes proposed in this PR. This led to the import itself being considered as internal, as after merging #1294 a scoped import is determined as external only if its path is external too.

Apart from removing the import name check from isExternalPath, this PR also makes import/external-module-folders more strict. It won't match incomplete path segments anymore, so some/path won't match /my/awesome/path but will still match /my/some/path and /my/some/path/index.js. Also, if an item starts with a forward slash, it will be treated as an absolute path prefix, so /home/me/project/packages will only match the directory itself and everything inside it.

This PR doesn't break any existing tests, and I believe that it shouldn't break any existing setups either.

skozin added 2 commits Jan 11, 2020
This test was added and skipped in #794 (probably since it was failing then), but it's not failing anymore on current master
Fixes #1597
@skozin

This comment has been minimized.

Copy link
Contributor Author

skozin commented Jan 12, 2020

CI build is probably failing due to the failing ESLint checks of the plugin source and test code, but these checks were as well failing before this PR.

@skozin skozin force-pushed the skozin:fix-order-for-monorepo-webpack-setup branch from 2bb01c1 to b59c495 Jan 12, 2020
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 12, 2020

Coverage Status

Coverage decreased (-0.04%) to 97.835% when pulling b4d5fd3 on skozin:fix-order-for-monorepo-webpack-setup into 71e87da on benmosher:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 12, 2020

Coverage Status

Coverage decreased (-4.2%) to 93.717% when pulling 2bb01c1 on skozin:fix-order-for-monorepo-webpack-setup into bcd9fe8 on benmosher:master.

@skozin

This comment has been minimized.

Copy link
Contributor Author

skozin commented Jan 12, 2020

Hmm, not sure why Coveralls reports that coverage of resolvers/node/index.js decreased, I didn't change or remove any tests or code related to that resolver.

@ljharb ljharb force-pushed the skozin:fix-order-for-monorepo-webpack-setup branch 2 times, most recently from 591aa56 to 89cde31 Jan 12, 2020
Copy link
Collaborator

ljharb left a comment

Looks great! just the one comment.

src/core/importType.js Outdated Show resolved Hide resolved
@skozin skozin requested a review from ljharb Jan 13, 2020
@skozin skozin force-pushed the skozin:fix-order-for-monorepo-webpack-setup branch from ed9c935 to 21404ec Jan 13, 2020
@ljharb ljharb force-pushed the skozin:fix-order-for-monorepo-webpack-setup branch from 21404ec to 9fac546 Jan 14, 2020
@ljharb
ljharb approved these changes Jan 14, 2020
@ljharb ljharb force-pushed the skozin:fix-order-for-monorepo-webpack-setup branch from 9fac546 to b4d5fd3 Jan 14, 2020
@ljharb ljharb merged commit b4d5fd3 into benmosher:master Jan 14, 2020
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
coverage/coveralls Coverage decreased (-0.04%) to 97.835%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@skozin skozin deleted the skozin:fix-order-for-monorepo-webpack-setup branch Jan 14, 2020
@skozin

This comment has been minimized.

Copy link
Contributor Author

skozin commented Jan 14, 2020

@ljharb, thank you so much for assisting me and merging this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.