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

fix: use deep merge strategy for context and options in Plugin #1009

Merged
merged 2 commits into from Jun 2, 2023

Conversation

xllily
Copy link
Contributor

@xllily xllily commented May 24, 2023

Initially, I attempted to modify the "source" order in Object.assign, but upon further consideration, I realized that utilizing a deep merge strategy would be a safer approach.

@xllily xllily force-pushed the fix/issue-997 branch 2 times, most recently from 087f332 to 2a32f1d Compare May 25, 2023 06:23
@xllily xllily changed the title fix(lib/plugin): change context and options merge strategy fix(lib): use deep merge strategy for context and options in Plugin May 25, 2023
@webpro
Copy link
Collaborator

webpro commented May 28, 2023

Thanks @xllily! Could you please give an example of what this change will fix? And given that Lodash is already a dependency, why did you choose to add a custom deepMerge?

@xllily
Copy link
Contributor Author

xllily commented May 30, 2023

Thanks @xllily! Could you please give an example of what this change will fix? And given that Lodash is already a dependency, why did you choose to add a custom deepMerge?

Thank you @webpro for your feedback! I apologize for not providing an example initially.

Regarding the issue with the log output in the terminal after a successful release, let me explain further. The example .release-it.json configuration you provided includes a custom registry URL (https://{{my-custom-host}}/) for npm publishing. However, despite this configuration, the log in the terminal still displays the default npm registry URL (https://www.npmjs.com/package/{{my-package}}).

I understand that this behavior is unexpected, and it aligns with the issue mentioned in #997. The purpose of my pull request was to address this issue and ensure that the log output in the terminal reflects the custom registry URL specified in the configuration.

As for the inclusion of the custom deepMerge implementation, I have realized that it may not be necessary in this case. I understand that Lodash is already a dependency, and its merge function can effectively handle the merging requirements. I have updated the pull request accordingly to reflect this realization.

Once again, thank you for your valuable feedback and guidance. I appreciate your assistance in improving the pull request, and I apologize for any confusion caused by my initial approach. Please let me know if there are any further concerns or suggestions.

@xllily xllily changed the title fix(lib): use deep merge strategy for context and options in Plugin fix: use deep merge strategy for context and options in Plugin May 31, 2023
@webpro webpro merged commit f0f0cf3 into release-it:main Jun 2, 2023
11 checks passed
@webpro
Copy link
Collaborator

webpro commented Jun 2, 2023

Thanks @xllily! This is great, happy to merge.

@webpro
Copy link
Collaborator

webpro commented Jun 2, 2023

🚀 This pull request is included in v15.10.4. See Release 15.10.4 for release notes.

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.

None yet

2 participants