Skip to content

[HOLD]chore: update changeset-snapshot-publish script to accept a parameter… #5366

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
"build:ts:watch": "wireit",
"build:types": "wireit",
"build:watch": "wireit",
"changeset-snapshot-publish": "yarn prepublishOnly && yarn changeset version --snapshot && yarn lint:versions --fix && yarn update-version && yarn changeset publish --no-git-tag --tag snapshot",
"changeset-snapshot-publish": "yarn prepublishOnly && yarn changeset version --snapshot ${1} && yarn lint:versions --fix && yarn update-version && yarn changeset publish --no-git-tag --tag snapshot",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just as a heads up, this kind of positional parameter does not translate well across operating systems. The most cross-system way to pass variables to commands is via a node script using yargs to read in from the command line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I appreciate your feedback about cross-platform parameter handling. You raise a valid point about positional parameters potentially causing issues across different operating systems.
While I agree that a Node script with yargs would be the most robust cross-platform solution, I believe this approach should work well for our current needs because:

  • Modern shells on macOS, Linux, and Windows (with appropriate tools) do support this type of parameter substitution
  • The Yarn/npm CLI handles these parameters consistently across platforms
  • This is specifically for snapshot releases, which we'll primarily be using for testing purposes
  • The implementation keeps all our command logic centralized in package.json
    The approach is straightforward and avoids the overhead of creating and maintaining separate script files
    Since this is just for snapshot/test releases rather than production deployments, the simplicity of this approach seems appropriate for our needs. If we encounter any compatibility issues, I'll certainly reconsider implementing a proper Node script solution. Thank you for the helpful suggestion!

Copy link
Collaborator

Choose a reason for hiding this comment

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

That all makes great sense! Thanks for outlining some of the background you did to validate this. This is only meant to be run in CI, is that correct? If we have anyone on a unique set-up, can we ask them to validate this locally if there's a chance it might need to be run manually?

"changeset-publish": "yarn prepublishOnly && yarn changeset version && yarn install && yarn lint:versions --fix && yarn update-version && yarn changeset publish --no-git-tag && yarn push-to-remote && yarn create-git-tag && yarn postpublish",
"update-version": "node ./tasks/update-version.js",
"chromatic": "chromatic --build-script-name storybook:build # note that --project-token must be set in your env variables",
Expand Down
Loading