-
Notifications
You must be signed in to change notification settings - Fork 22
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
chore: use radashi-helper
#220
chore: use radashi-helper
#220
Conversation
.devcontainer/devcontainer.json
Outdated
@@ -8,17 +8,23 @@ | |||
// Features to add to the dev container. More info: https://containers.dev/features. | |||
"features": { | |||
"ghcr.io/devcontainers-contrib/features/pnpm": { | |||
"version": "9.1.3" | |||
"version": "9.8.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We pin the PNPM version to v9.1.3 in our package.json for reproducible builds and lockfile consistency. I've tried using 9
or 9.x.x
, but apparently an exact version is required for the packageManager
field.
https://github.com/radashi-org/radashi/blob/main/package.json#L15
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the version more because of the warnings that appeared to me when I was going to install a new dependency. But really, it makes more sense to keep the same as package.json
.devcontainer/devcontainer.json
Outdated
"ghcr.io/devcontainers-contrib/features/npm-package": { | ||
"package": "radashi-helper", | ||
"version": "latest" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add radashi-helper
to the package.json instead, so it's available even to people not using dev containers.
Probably best not to use latest
as it might increase risk of supply chain attacks if one of our transient dependencies ever gets compromised. Once we have Renovate up and running, periodic updates should be good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add it, but even when Radashi is included only as a development dependency, we still can't use the radashi
keyword before adding pnpm
prefix or similar. By configuring this in the Dev Container, anyone using it won't need to run pnpm radashi *
.
Of course, there are some workarounds we could implement only with the package.json, but in this case, it would be better to just use pnpm
prefix.
About the version, I agree with you.
}, | ||
"postCreateCommand": "pnpm i", | ||
"postStartCommand": "pnpm i", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between postCreate and postStart here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both commands are similar, but postCreateCommand
runs during the container's initial setup, while postStartCommand
is only executed once the container is fully functional. For me, it makes more sense to install dependencies in postStartCommand
, ensuring the container is ready. In this current scenario, it doesn't make a difference, but I updated it to make it more semantic.
https://containers.dev/implementors/json_reference/#lifecycle-scripts
Btw, I'm not sure if it's GitHub's doing or not, but your video/gif demonstration is very low resolution. Hard to see anything. |
Yes, the compression makes the gif have a poor quality. I have the video link for you. |
Tip
The owner of this PR can publish a preview release by commenting
/publish
in this PR. Afterwards, anyone can try it out by runningpnpm add radashi@pr<PR_NUMBER>
.Summary
Recently, I noticed some activity in this repository. I immediately wanted to set up the project so that these tools come pre-installed.
Related issue, if any:
For any code change,
Does this PR introduce a breaking change?
No