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

feat: add ignore option to extraneous deps #2039

Closed
wants to merge 1 commit into from

Conversation

Aghassi
Copy link
Contributor

@Aghassi Aghassi commented Apr 27, 2021

We are doing a migration with 1st party dependencies in a monorepo that requires this ability, otherwise we get false positives. This option allows the user to pass a regex so that if a package matches the regex it won't report as an offender.

We are doing a migration with 1st party dependencies in a monorepo that requires this ability, otherwise we get false positives. This option allows the user to pass a regex so that if a package matches the regex it won't report as an offender.
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 elaborate on why this is needed?

@@ -200,6 +204,7 @@ module.exports = {
'peerDependencies': { 'type': ['boolean', 'array'] },
'bundledDependencies': { 'type': ['boolean', 'array'] },
'packageDir': { 'type': ['string', 'array'] },
'ignore': { 'type': 'regexp' },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a very bad idea to allow regex strings in eslint configs; that's how you get CVEs. this should be a glob string instead.

i'd also expect it to be allowed to be an array of glob strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's a good idea!

@ljharb
Copy link
Member

ljharb commented Apr 27, 2021

Also, see #903.

@Aghassi
Copy link
Contributor Author

Aghassi commented Apr 27, 2021

@ljharb Sure thing. We are migrating our codebase to bazel, and as an interim step for users we use a preinstall script to generate the bazel built versions of 1st party dependencies. We then specify the built artifacts as part of a workspace to avoid dealing with symlinking oddities under yarn. However, we are not using the workspace in the normal sense since we just have a single package.json that manages the projects while we are in this migration state. As such, we never declare the packages in the dependencies object, but they are guaranteed to be there. The workspace field is currently ignored by this rule (and also is not structure in a standard way that we can make assumptions around it IMO), so I added the ignore option as a way to eject for first party things using our companies @ scope. Maybe this isn't a use case for everyone, but I wanted to at least start discussion of why we needed this. I'm using patch-package internally to do this for now, so if it isn't taken in upstream that's fine. I wanted to open a draft PR to at least show what we had done and leave a paper trail 😄 . I'll read through those issues.

@ljharb
Copy link
Member

ljharb commented Apr 27, 2021

I'm confused; bazel has workspaces support (using links). What oddities?

@Aghassi
Copy link
Contributor Author

Aghassi commented Apr 27, 2021

I'm confused; bazel has workspaces support (using links). What oddities?

I think we are crisscrossing the word workspaces. Just to confirm, we are talking about yarn workspaces. And in the above, I should clarify that we are not yet running the main projects under bazel, but we are generating dependencies that these projects rely on under bazel. So essentially we are giving developers a local experience that is still based in the NPM ecosystem, but linking first party artifacts that bazel generated. The issues were hit were when you use link: yarn fails to install because of missing node_modules if I recall. Using file: sort of solves the problem, but then it doesn't get updated when you reinstall over existing node_modules folder. It's quite possible we are going about this the "wrong" way for now, but using yarn workspaces has removed all caching invalidation issues and linking issues of the bazel built artifacts to these projects that aren't under bazel yet.

Does that make sense? I'm probably doing a poor job of explaining it :/

@shellscape
Copy link

I dropped a quick comment on a very common ecosystem use case that demonstrates the need for this feature here: #903 (comment)

@Aghassi
Copy link
Contributor Author

Aghassi commented Oct 7, 2021

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.

None yet

3 participants