Skip to content

Use resolved package name #1119

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 4 commits into from
Closed

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?

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

mieszko4 added 2 commits June 12, 2018 16:01
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 requested review from benmosher and jfmengels June 12, 2018 23:08
@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.

@ljharb
Copy link
Member

ljharb commented Aug 30, 2024

@mieszko4 this needs to be rebased. i think that the part you'll need to update now is either checkDependencyDeclaration or getModuleOriginalName.

@ljharb ljharb marked this pull request as draft August 30, 2024 17:51
@mieszko4
Copy link
Author

mieszko4 commented Sep 1, 2024

It's been 6+ years since I created this PR.
I haven't had a need for this to be merged. And I am not sure if this PR will resolve the mentioned issue.

I think that we should just close this PR. In the future would be great if PRs could be closed or merged quicker :)

@ljharb
Copy link
Member

ljharb commented Sep 1, 2024

@mieszko4 that's always great, but we're all unpaid volunteers here, and things take the time they take :-)

I'll close it then, thanks.

@ljharb ljharb closed this Sep 1, 2024
@mieszko4 mieszko4 deleted the patch-1 branch September 1, 2024 22:32
@mieszko4
Copy link
Author

mieszko4 commented Sep 1, 2024

@mieszko4 that's always great, but we're all unpaid volunteers here, and things take the time they take :-)

Yeah, I totally get this. I also would check it in more detail if I had time :). Anyway, no hurt feelings, no worries.

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

Successfully merging this pull request may close these issues.

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