feat(publish): make npm-publish apply publishConfig overrides, closes #404 #410
Conversation
Codecov Report
@@ Coverage Diff @@
## main #410 +/- ##
==========================================
- Coverage 97.31% 97.09% -0.21%
==========================================
Files 146 146
Lines 4335 4353 +18
Branches 1004 1008 +4
==========================================
+ Hits 4218 4226 +8
- Misses 117 127 +10
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| 'os', | ||
| ]; | ||
|
|
||
| async function overwritePublishConfig(manifestLocation: string) { |
There was a problem hiding this comment.
I would prefer "override" instead of "overwrite" because the latter can be confused by thinking it's overwriting the entire file which is far from the reality. Also, pnpm wrote something similar in their docs
It is possible to override some fields in the manifest before the package is packed. The following fields may be overridden:
| @@ -69,6 +73,47 @@ export async function packDirectory(_pkg: Package, dir: string, options: PackCon | |||
| ); | |||
| } | |||
|
|
|||
| export const PUBLISH_CONFIG_FIELDS = [ | |||
There was a problem hiding this comment.
it looks like there's also a "typesVersion" that pnpm can also override as defined in their docs
https://pnpm.io/package_json#publishconfig
| PUBLISH_CONFIG_FIELDS.forEach((key) => { | ||
| if (key in publishConfig) { | ||
| manifest[key] = publishConfig[key]; | ||
| delete publishConfig[key]; |
There was a problem hiding this comment.
I didn't test the code but I wonder what's the reason that you have to delete the object property? I see you're also deleting a few lines below as well
|
nice PR, I wonder if we should put this under a flag though? Perhaps an inverse flag in case some users might want to disable it. Also does npm support that? They probably don't right? So I wonder if we should run this function only when it's pnpm or yarn defined as It also looks like all the code from the new function you added isn't tested, we'll need to make sure these are covered. I see that it does go in the
|
ghiscoding
changed the title
|
Thanks for your comment. I think providing through options flag is a good idea. I will reflect you reviews this weekend. Thanks! |
ghiscoding
changed the title
|
@artechventure I had a chance to take a look at your PR today. After looking at the PR and trying to add unit tests, I discovered the code you added in I'll be closing this PR, but don't worry you will still be a contributor since I forked your PR 😉 |
Description
Make npm-publish function to ovveride manifest by publishConfig.
Motivation and Context
#404
As described in an issue above, yarn and pnpm has it's own logic to overwrite publishConfig to manifest.
So I added that logic into npm-publish function.
Added ones doesn't effect existing publishConfig related logics.
How Has This Been Tested?
Added an test code.
Types of changes
Checklist: