-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix import type determination for monorepo setup w/ webpack resolver #1605
Conversation
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
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. |
2bb01c1
to
b59c495
Compare
Hmm, not sure why Coveralls reports that coverage of |
591aa56
to
89cde31
Compare
There was a problem hiding this 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.
ed9c935
to
21404ec
Compare
21404ec
to
9fac546
Compare
9fac546
to
b4d5fd3
Compare
@ljharb, thank you so much for assisting me and merging this! |
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 theimport/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: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
toimport/external-module-folders
array. However, this didn't work in a project that uses thewebpack
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, soisExternalPath
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 makesimport/external-module-folders
more strict. It won't match incomplete path segments anymore, sosome/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.