fix: provide a better experience for patch_args when pnpm patches are used #837
Conversation
npm/npm_import.bzl
Outdated
| # TODO (2.0): Stop defaulting patch_args with {"*": ["-p0"]} when no patches are supplied | ||
| # as this conflicts with the default pnpm.patchedDependencies format of "-p1" and | ||
| # needs to be explicitly overridden by the user. | ||
| if len(patches) == 0: | ||
| patch_args = {} | ||
|
|
There was a problem hiding this comment.
I figure this isn't a breaking change because it won't affect anyone who doesn't have patches.
|
Did an issue come up with this or is it just confusing / a bad API now that the pnpm one has a different default? |
I just noticed that I have to override |
|
Alternatively, what I could do is have patches ignore the |
Aren't there other patch args that the pnpm patches might need? |
pnpm patches have to be |
|
Aren't there other arguments though? I thought it was a generic array of arguments and not only the |
None that are defaulted in the macro. |
|
Oh I see what you mean @jbedard . I shouldn't remove any non-strip args here. I'll do another pass. |
Actually no, if there are no |
|
But now if patches are added then the default args for pnpm suddenly change. I don't think adding an entry to |
Yeah, that's true. I think there might be another way to solve this. Maybe "*" patch_args just shouldn't apply to pnpm patches (at least any strip args in there). |
|
Is the pnpm format a default of "-p1" but it can be changed? Maybe aligning our default with pnpm is best then? |
Cannot change the format for pnpm patches, and changing our default would be a breaking change. |
|
I guess not applying Do we need to apply the patch args to pnpm at all? Normally with pnpm how are patch args applied? Why do we have to add our own patch args? |
It's just because pnpm patches and |
So it would be hard to treat pnpm different and ignore the patch args? |
I think it's doable. I'll do a pass when I get back to this. |
gregmagolan
added
the
wip
kormide
changed the title
|
Finally got back to this. I improved the error message when a strip prefix is set for |
| patch_args = { | ||
| "*": ["-p1"], | ||
| }, |
There was a problem hiding this comment.
The patch arg has to be -p1 for this e2e where pnpm.patchedDependencies is used, so this exclusion exercises the logic where our default of -p0 is overridden.
kormide
added
enhancement
| # requirement of "-p1" for patches. When pnpm patches are used, it's highly likely that | ||
| # `patches` is unset, so when it is empty remove any strip prefixes from the default or | ||
| # any user-supplied argument as they won't have an effect anyway. | ||
| if len(patches) == 0 and patch_args: |
There was a problem hiding this comment.
Would it be possible and any nicer changing the default to patch_args = None and adding the default when there are patches instead of clearing it when no patches? Mainly to avoid the extra strip method...
def npm_translate_lock(
name,
....
patches = {},
patch_args = None,
...
if len(patches) > 0 and patch_args == None:
patch_args = {"*": ["-p0"]}
There was a problem hiding this comment.
My intention to perform the substitution whether patch_args is defaulted or the user supplies a value. As long as patches is empty, the strip prefix should be irrelevant because we always override the pnpm patches with -p1.
There was a problem hiding this comment.
Why would patch_args be set if patches is empty though?
There was a problem hiding this comment.
It could be. Technically someone could set non-strip args (I'm not sure what those are) and they would apply to the pnpm patches.
There was a problem hiding this comment.
Then they'd be overriding the None default and we wouldn't have to do anything though?
There was a problem hiding this comment.
That's what we do currently and it causes issues with the default -p0 when using pnpm patches which prefer the -p1
There was a problem hiding this comment.
I mean change how our defaults work. Different defaults depending if we are using patches or assuming we're using pnpm (because patches is empty). Different defaults in these 2 cases instead of modifying/converting the args in 1/2 cases.
If that makes sense?
There was a problem hiding this comment.
It's hard to document in that case since stardoc wouldn't show the default.
I think it is cleaner to document that -p1 is forced for any patches that are listed in the pnpm package.json patches and both the default and any user supplied patch_args for -p or --strip are ignored in that case.
There was a problem hiding this comment.
Can you think of a way of doing that without the current _remove_strip_prefix_arg() implementation though?
Scanning args for various -p* or --strip=* names and modifying the args is just something I'd prefer avoiding, especially with almost no test coverage :/
There was a problem hiding this comment.
I don't have any better ideas besides that. Maybe just appending -p1 is sufficient if patch lets the last strip argument win but we'd have to test that.
|
Done with #1694 |
…_lock(patches) (aspect-build#1694) Fix aspect-build#1427 Close aspect-build#1693 Close aspect-build#837
…_lock(patches) (aspect-build#1694) Fix aspect-build#1427 Close aspect-build#1693 Close aspect-build#837
Revisiting this change as it was brought up recently in the Bazel slack group: https://bazelbuild.slack.com/archives/CEZUUKQ6P/p1691179585032929