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

Rewrite internals for v2 release #83

Merged
merged 36 commits into from
Apr 20, 2023
Merged

Rewrite internals for v2 release #83

merged 36 commits into from
Apr 20, 2023

Conversation

mcous
Copy link
Member

@mcous mcous commented Apr 14, 2023

Overview

Work in progress!

This PR is a large-scale rewrite of the module, primarily concerned with fixing all outstanding (known) bugs, and grabbing a few easy features:

It will also lay the groundwork for #81, which can be tackled in its own PR.

Details

The core of this PR is a ground-up rewrite of how this module interacts with npm.

Previously, this action would read and then overwrite npm's user config (aka ~/.npmrc). This is a harmful thing to do! It interferes with configuration written by actions/setup-node, and if you happen to use the CLI version of the package, it would overwrite your own settings. See #15 for more details.

This PR changes the logic to write (and delete when we're done) an isolated .npmrc file in temporary storage, specific to this package. The action will no longer interfere with other tools, and other tools won't be able to interfere with this action.

Since the npm interaction code was being rewritten, it was easy to layer in some additional changes:

  • Use the CLI's <package_spec> argument instead of changing the working directory
  • Always pass --ignore-scripts to the CLI

On top of how the module calls npm, this PR also takes a top-down TDD approach to all internal interfaces and functions. A lot of the other fixes and changes fell out of that work, including:

  • Rewrite manifest reading to also check publishConfig
  • Move release version checking to its own submodule with better test coverage
    • e.g. check against all published versions, not just `dist-tags
  • Replace check-version and greater-version-only options with a single strategy option that can be set to upgrade (default) or any
    • strategy: upgrade -> check-version: true, greater-version-only: true
    • strategy: any -> check-version: true, greater-version-only: false
    • Removed check-version: false, because you shouldn't be using this action if you don't want to check against published versions; you can just use npm

@mcous mcous marked this pull request as ready for review April 19, 2023 02:03
@mcous mcous merged commit c5f37b9 into main Apr 20, 2023
@mcous mcous deleted the replace-npm-submodule branch April 20, 2023 02:34
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.

1 participant