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 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.

This test was added and skipped in import-js#794 (probably since it was failing then), but it's not failing anymore on current master
@skozin
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 fix-order-for-monorepo-webpack-setup branch from 2bb01c1 to b59c495 Compare January 12, 2020 00:48
@coveralls
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
Copy link

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
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 fix-order-for-monorepo-webpack-setup branch 2 times, most recently from 591aa56 to 89cde31 Compare January 12, 2020 21:50
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! just the one comment.

src/core/importType.js Outdated Show resolved Hide resolved
@ljharb ljharb force-pushed the fix-order-for-monorepo-webpack-setup branch from 9fac546 to b4d5fd3 Compare January 14, 2020 06:42
@ljharb ljharb merged commit b4d5fd3 into import-js:master Jan 14, 2020
@skozin skozin deleted the fix-order-for-monorepo-webpack-setup branch January 14, 2020 14:29
@skozin
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
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect behavior of import/order in a monorepo setup
3 participants