Add ability to ignore peer depdenencies from package.json #1163
Conversation
bweston92
requested review from
alexeagle,
gregmagolan and
soldair
as code owners
| findDeps(dep.dependencies, () => true, 'dependency'); | ||
| findDeps(dep.peerDependencies, (targetDep) => { | ||
| // if it is not in the can ignore then is is required | ||
| return canIgnore.indexOf(targetDep) === -1; |
There was a problem hiding this comment.
so the field canIgnore is a list in the user's package.json? a new field you're adding?
Would be good to have some doc change so users can figure out how to use this, along with some test (can just be that you find a way to reproduce the failure in this repo by not installing a peer dep, then use your new feature to fix that breakage)
There was a problem hiding this comment.
Yes, canIgnore is a string list in the package.json, I was going to add documentation but I don't know whether or not to do it this way, we also have an alternative of putting in a rule attribute, which would you prefer?
There was a problem hiding this comment.
I'd prefer an attribute over a new key added to the package.json
There was a problem hiding this comment.
@Toxicable I personally think I am with you, it was something I thought about after I wrote this but will amend asap.
There was a problem hiding this comment.
yeah it's a behavior of npm_install/yarn_install so seems like it should go there. I think the default should be to act like npm and yarn which means we should print peerDep warnings but proceed. Offer users a strictPeerDependencies boolean option. (I guess you could have a list of specific peerDep warnings to whitelist in strict mode)
There was a problem hiding this comment.
I have changed the way it is done, will check when at work again tomorrow to make sure it still works if so I will add documentation if it is ok with you?
internal/npm_install/npm_install.bzl
Outdated
| @@ -89,6 +89,10 @@ which are installed, but failures on installation are ignored. | |||
| "exclude_packages": attr.string_list( | |||
| doc = """DEPRECATED. This attribute is no longer used.""", | |||
| ), | |||
| "ignore_peer_dependencies": attr.string_list( | |||
| default = [], | |||
| doc = """A list of peer depedencies that can be ignored.""", | |||
There was a problem hiding this comment.
Could you add to this doc that strict_peer_dependencies must be set for you to use this?
Or maybe change the logic a little bit so that you can set this without needing strict_peer_dependencies
There was a problem hiding this comment.
How would you suggest a change the logic which can have the following 3 use cases:
a = strict_peer_dependencies
b = ignore_peer_dependencies
- You want to ignore peer dependencies. (
a = False,b = []) - You want to have strict dependencies and ignore a few of them. (
a = True,b = ['dep1', 'dep2']) - You want to have strict dependencies and ignore none of them. (
a = True,b = [])
There was a problem hiding this comment.
Can you find any other popular tooling that gives a strict mode for peer deps? I would like to follow some precedent rather than invent something new.
If there is no precedent at all, maybe we shouldn't even have a strict mode - that should be someone else's job, orthogonal to whether the package manager is run by bazel
There was a problem hiding this comment.
I don't really know of any, Yarn and NPM both give warning but nothing more.
There was a problem hiding this comment.
Yeah tha's a good point @alexeagle I don't know of any that have strict deps.
Maybe we should just remove the checks, if anything it should be yarn checking things like this right, not us?
There was a problem hiding this comment.
Ok so should I revert it all and put the “true” to “false” for required?
There was a problem hiding this comment.
I have reverted and set the peer dependencies as a non requirement altogether. Is that ok?
There was a problem hiding this comment.
Yeah sounds good to me, sorry for all the back and forward.
We appciate you taking the time here!
|
Could you rebase past the conflict, squash commits and update commit message to pass the .commitlint check (seems like your precommit hooks are not working?) |
|
@alexeagle is that ok? |
|
I think you've done something wrong, did you look at the resulting diff? This PR now proposes editing only two lines which can't be right. I guess I'll grab your earlier snapshot and just fix this for you... |
|
That is right, before we wanted to allow us to ignore some of the peer dependencies, now we're just turning off the flag which makes peer dependencies required in the first place. |
bweston92
deleted the
feature/1162-add-ability-to-ignore-peer-dependencies
branch
|
For anyone struggling, it seems that optionalDependencies in your package.json works to bypass it.
|
|
@httpete yes it does since this PR, prior to this PR peerDepedencies where strictly required by rules_nodejs. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #1162
What is the new behavior?
Ability to pass a
canIgnorelist of strings containing peer dependency names that can be ignored.Does this PR introduce a breaking change?
Other information
This is one implementation, we could make it so we can pass it via the
*_installrule instead if it might be cleaner.