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(publish): apply publishConfig overrides, closes #404 #415

Merged
merged 7 commits into from Nov 19, 2022

Conversation

ghiscoding
Copy link
Member

@ghiscoding ghiscoding commented Nov 18, 2022

supersede PR #410, closes #404

Description

It is possible to override some fields in the manifest before the package is published by defining them in publishConfig, this PR is both a fix (for #404) and a feature.

Motivation and Context

resolves issue #404, which was that certain fields defined in publishConfig are defined as overrides for the final package.json to be published. Lerna-Lite should take these in consideration and apply the override when it exists as per pnpm publishConfig documentation

How Has This Been Tested?

Added necessary unit tests with all possible variances of overrides

Types of changes

  • Chore (change that has absolutely no effect on users)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov
Copy link

codecov bot commented Nov 18, 2022

Codecov Report

Merging #415 (4d6bfe9) into main (ec49df4) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #415      +/-   ##
==========================================
+ Coverage   97.31%   97.32%   +0.02%     
==========================================
  Files         146      147       +1     
  Lines        4335     4356      +21     
  Branches     1004     1009       +5     
==========================================
+ Hits         4218     4239      +21     
  Misses        117      117              
Impacted Files Coverage Δ
...kages/cli/src/cli-commands/cli-publish-commands.ts 100.00% <ø> (ø)
packages/cli/src/lerna-entry.ts 100.00% <100.00%> (ø)
packages/core/src/package.ts 100.00% <100.00%> (ø)
packages/core/src/utils/object-utils.ts 100.00% <100.00%> (ø)
...ackages/publish/src/lib/override-publish-config.ts 100.00% <100.00%> (ø)
packages/publish/src/publish-command.ts 99.06% <100.00%> (+0.02%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ghiscoding
Copy link
Member Author

ghiscoding commented Nov 18, 2022

@artechventure if you could review this and maybe give it a try on your side, that would be great. It should work as it is, but I'll look into doing a dry-run test tomorrow to make sure it's all good before merging.

I'd like to release this quite soon, I got other PRs waiting to be released

EDIT

I rewrote the code and decided to use the exact same code as pnpm uses (ref pnpm overridePublishConfig.ts), so the override will end up being exactly the same as pnpm.

I also finished testing in dry-run mode with temp files, it looks all good. Unless I get any news back, I will probably merge in next couple days.

await gitCommit(cwd, 'setup');
};

it('overrides npm publish with publishConfig that are valid and leave fields that are not in the whitelist to be untouched and remain in publishConfig', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM 👍
minor) 'Property based testing' like fastcheck seems to be very fit in these cases. After merging, can I open another PR and add some more test cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what you mean with fastcheck (I don't want to add more deps), but sure you can add more tests once I merge it. I decided to go with the exact same code as pnpm (see here), so that it should do exactly the same as what pnpm would perform

Also, just so you know, I thought that we had to merge object properties when the publishConfig provides only a partial object in comparison to what is in the root but looking at pnpm implementation it seems to override (reassign) the entire property every time and not try to merge partial object. At the end I want to have the same logic as pnpm and so it makes sense to copy their code implementation

@ghiscoding ghiscoding merged commit 03e8157 into main Nov 19, 2022
@ghiscoding ghiscoding deleted the feat/publish-config-overrides branch November 19, 2022 22:40
@ghiscoding
Copy link
Member Author

ghiscoding commented Nov 22, 2022

@artechventure this is now available in new v1.13.0 release. Thanks for the contribution & feedback 🤝

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.

'lerna publish' doesn't respect publishConfig
2 participants