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

Use resolved package name #1119

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Use resolved package name #1119

wants to merge 4 commits into from

Conversation

mieszko4
Copy link

@mieszko4 mieszko4 commented Jun 8, 2018

Should fix #496

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.

Can you add a test case that would have failed without this change?

src/rules/no-extraneous-dependencies.js Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jun 12, 2018

Coverage Status

Coverage increased (+2.8%) to 97.268% when pulling e25180c on mieszko4:patch-1 into ebafcbf on benmosher:master.

Assume folder with packages is node_modules and take part of path after last node_modules
@mieszko4
Copy link
Author

@ljharb I've added the test that would have failed without fix in no-extraneous-dependencies.js. Please take a look.

@ljharb ljharb added the bug label Jun 12, 2018
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.

I'm a little nervous about letting node_modules sneak into a rule proper, though to be fair no-extra-deps is a Node-specific rule.

I'll try to come back around to this in a second pass after letting it cook in my brain for a little bit.

@ljharb
Copy link
Member

ljharb commented Mar 4, 2020

@arcanis i'm intending to merge this soon, does it need any changes to avoid causing berry problems?

@arcanis
Copy link
Contributor

arcanis commented Mar 4, 2020

From the look of it it should be compatible in most cases, since we store packages inside a standalone node_modules path (to make vendor detection easier):

/app/.yarn/cache/foo-1.0.0.zip/node_modules/foo/index.js

One caveat however: we cannot do that when people use the workspace:, link:, or portal: protocol, because they are resolved to the final path. So given the package.json:

{
  "dependencies": {
    "foo": "link:./foo"
  }
}

You'd have /app/foo, not /app/node_modules/foo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

no-extraneous-dependencies: aliases are not considered "project"-level imports
5 participants