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

Add target option #69

Closed
wants to merge 1 commit into from
Closed

Conversation

razor-x
Copy link
Collaborator

@razor-x razor-x commented Jan 2, 2023

Allows passing the first argument to npm publish as a new option: target.

This is particularly useful if building the packing or installing dependencies takes a lot of time but is available as an artifact in an earlier job.

Build is passing when rebased off this PR: #68

Closes #59

@razor-x razor-x marked this pull request as ready for review January 2, 2023 04:19
@mcous
Copy link
Member

mcous commented Apr 7, 2023

@razor-x can you describe how target differs from the existing package option? Are they both required? It's looking like the bump to Node v16 will require a v2 anyway, so if there's an opportunity to drop some unnecessary options that would be cool!

Ideally I'd hope that we only need target and not package

@razor-x
Copy link
Collaborator Author

razor-x commented Apr 7, 2023

@mcous Yes, I guess the arg name should be package-spec to match the npm docs. It could also be package for short (but that was already taken).

One could argue that the current arg package is misnamed, as it really is more the current working directory in the existing implementation.

Anyway, the important thing is to have a way to pass in the first (and only) argument to npm publish from the action. Options I'm thinking of in order of 1 (worst) 3 (best)

  1. Keep this as-is. (I don't really like target anymore as a name.)
  2. Rename target to package-spec: matches the docs, but confusing with the existing package option.
  3. Introduce package-spec (non-breaking). Rename package to working-directory (breaking). Can be done in two incremental steps and does not require a breaking change to have the feature released.

@mcous
Copy link
Member

mcous commented Apr 7, 2023

@razor-x I'm here for option (3). I think it's nice in that is also addresses #51, because calling it working-directory makes it really clear that you're overriding the default GH Actions behavior

@mcous
Copy link
Member

mcous commented Apr 10, 2023

I've been working my way through the code in the main branch, and I'm starting to have a few thoughts about the existing package and proposed package-spec inputs:

  • I'm not sure overriding the working directory is important or useful, compared to exposing the package-spec arg (or a workspace option, in the future), so we can probably drop the idea of working-directory for now
  • If the package-spec is a pre-packed tarball, then we still need something to point us to package.json so we can read the package name / current version.
    • Or maybe we extract package.json from the archive?
  • If package-spec is a directory, we should use the package.json in the directory, I think

What do you think of these points? To brainstorm something:

  • Keep the package arg
  • If package unspecified, default to regular npm behavior: use the cwd as the current package
  • If package is a package.json file, use the directory of that file as the package_spec argument to npm
  • If package is a directory, look for a package.json in that directory and use the directory as the package_spec arg to npm
  • If package is an archive, I'm not sure
    • Extract package.json from the archive?
    • Require a separate package-manifest input?

@razor-x
Copy link
Collaborator Author

razor-x commented Apr 10, 2023

I'm not sure overriding the working directory is important or useful, compared to exposing the package-spec arg (or a workspace option, in the future), so we can probably drop the idea of working-directory for now

Agreed. Especially since working directory is supported by GitHub actions, probably best not to mess with it.

If the package-spec is a pre-packed tarball, then we still need something to point us to package.json so we can read the package name / current version. Or maybe we extract package.json from the archive?

Good point, the way I was using this I was assuming the correct package.json would be in the current working directory, but that is just a lucky assumption.

What do you think of these points? To brainstorm something:...

That looks like the right logic to me. And yes, we should extract the package.json file from the tar file, not require another argument. I don't think we need to extract it to the file system though, we should just be able to read it into memory.

@razor-x
Copy link
Collaborator Author

razor-x commented Apr 14, 2023

@mcous Closing this one since with the new proposal we would not be adding a new input but changing the behavior of the exiting package input.

Thanks for bringing this repo back from the brink, the updates coming in look great! I am really busy recently, and I can try to get back to opening a new PR with those changes, but if you get to it first it's ok with me!

Moved to issue: #81

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.

Configure publish directory
2 participants