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

Pass ignoredWorkspacePatterns to mapWorkspaces ignore param (resolves… #374

Closed
wants to merge 1 commit into from

Conversation

arnoud-dv
Copy link
Contributor

@arnoud-dv arnoud-dv commented Nov 29, 2023

Pass the ignoreWorkspaces patterns to the ignore param of mapWorkspaces to resolve #373

Note that I did test that this prevents the error from occurring but did not perform other testing to make sure nothing breaks.

Copy link

vercel bot commented Nov 29, 2023

Someone is attempting to deploy a commit to a Personal Account owned by @webpro on Vercel.

@webpro first needs to authorize it.

@webpro
Copy link
Collaborator

webpro commented Nov 30, 2023

Thanks, @arnoud-dv. Sorry you have to go through all this :) I'm not against this PR per se, but that error message is not coming from Knip.

That would be "Duplicate package name" (https://github.com/webpro/knip/blob/main/packages/knip/src/ConfigurationChief.ts#L323)

So I'm a bit surprised how modifying Knip's behavior would get you past that error?

(Apparently there's also something else saying it's not great to have duplicate workspace names...)

@arnoud-dv
Copy link
Contributor Author

Hi, did you see the issue I added for this? I describe it more in depth there.

@webpro
Copy link
Collaborator

webpro commented Nov 30, 2023

Yes, that's why I said the "must not have multiple workspaces with the same name" error message is not from Knip.

@webpro
Copy link
Collaborator

webpro commented Nov 30, 2023

This change seems to break the feature that shows configuration hints for ignoredWorkspaces that don't need to be ignored (e.g. because the folder is no longer there).

@arnoud-dv
Copy link
Contributor Author

Do you think that's fixable? This might be a blocker to get the Angular adapter merged to Tanstack Query. I cannot do much about how Angular packages are built.

@webpro
Copy link
Collaborator

webpro commented Nov 30, 2023

Hmm.. tbh I don't really see a way, other than re-implementing what mapWorkspaces does, which defeats the point. I don't want to run mapWorkspaces twice (once with a try/catch, etc).

(And I want Knip to throw for duplicate package names, because not having that leads to hard to debug issues, just recently ran into this from another angle: #339)

Kinda out of ideas, sorry 🤷‍♂️

@arnoud-dv
Copy link
Contributor Author

arnoud-dv commented Nov 30, 2023

While looking at the Knip code and considering that it shouldn't break the configuration hints I couldn't think of a solution either.

I think I solved it for my PR though: the pnpm-workspace.yaml of TanStack Query contains double asterisk globs

packages:
  - 'packages/**'

Changing these to single ones fixes the problem and I think is actually an improvement because no real packages are nested inside other directories.

Knip's pretty cool btw! 😊

@webpro
Copy link
Collaborator

webpro commented Dec 1, 2023

While looking at the Knip code and considering that it shouldn't break the configuration hints I couldn't think of a solution either.

I think I solved it for my PR though: the pnpm-workspace.yaml of TanStack Query contains double asterisk globs

packages:
  - 'packages/**'

Changing these to single ones fixes the problem and I think is actually an improvement because no real packages are nested inside other directories.

You can use negations there too, btw:

packages:
  - 'packages/**'
  - '!packages/not-this-one'
  - '!packages/**/build/**'

Knip's pretty cool btw! 😊

Thank you!

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

Successfully merging this pull request may close these issues.

Package.json files having the same name property results in error even when added to ignoreWorkspaces
2 participants