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

feat: publish to npm #159

Merged
merged 31 commits into from
Apr 16, 2024
Merged

feat: publish to npm #159

merged 31 commits into from
Apr 16, 2024

Conversation

dbolson
Copy link
Contributor

@dbolson dbolson commented Apr 10, 2024

Set up npm to publish package that wraps binary.

I've tested this with the manual release workflow, but we'll need to test it once it's merged.

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Related issues

Provide links to any issues in this repository or elsewhere relating to this pull request.

Describe the solution you've provided

Provide a clear and concise description of what you expect to happen.

Describe alternatives you've considered

Provide a clear and concise description of any alternative solutions or features you've considered.

Additional context

Add any other context about the pull request here.

@dbolson dbolson changed the title Sc 234872/publish to npm feat: publish to npm Apr 10, 2024
@dbolson dbolson force-pushed the sc-234872/publish-to-npm branch 3 times, most recently from cc2d90e to bff6f70 Compare April 10, 2024 21:10
@@ -14,7 +14,7 @@ jobs:
build:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bumping this for consistency.

@@ -39,6 +39,50 @@ jobs:
token: ${{ secrets.GITHUB_TOKEN }}
homebrew-gh-secret: ${{secrets.HOMEBREW_DEPLOY_KEY}}
tag: ${{ inputs.tag }}

release-ldcli-npm:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be the same as what's in the release-please action.

fetch-depth: 0
- uses: actions/setup-node@v3
with:
node-version: 20.x
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bumped to 20.x because of npm warning.

.yarnrc.yml Outdated
@@ -0,0 +1,13 @@
nodeLinker: node-modules
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 don't know what anything in this directory and the .yarn directory does. I copied these from the js-core repo.

Copy link
Contributor Author

@dbolson dbolson left a comment

Choose a reason for hiding this comment

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

@kinyoklion I don't have a good understanding of what's going on in here. I pattern-matched much of the code from the js-core repo and kept tweaking it until it worked. Would you, or someone else you suggest, have time to review this and maybe talk through what's going on and if we could simplify it?

@kinyoklion
Copy link
Member

@kinyoklion I don't have a good understanding of what's going on in here. I pattern-matched much of the code from the js-core repo and kept tweaking it until it worked. Would you, or someone else you suggest, have time to review this and maybe talk through what's going on and if we could simplify it?

I think that we could simplify things. The js-core repo publishes 15 packages for several ecosystems, so there is more going on than is strictly needed.

Though there can be some benefits from consistency.

I would be happy to talk through things, but I probably wouldn't be able to do that until Monday.

{
"name": "@launchdarkly/ldcli",
"description": "The official command line interface for managing LaunchDarkly feature flags.",
"version": "0.0.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This gets overwritten similar to the go version variable.

@@ -0,0 +1,9 @@
cmd/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need to include these files in the npm package.

@@ -5,7 +5,14 @@
"bump-minor-pre-major": true,
"versioning": "always-bump-minor",
"bootstrap-sha": "f37bd3aee1f117faba005bf96f0d0888f898ee41",
"prerelease": true
"prerelease": true,
"extra-files": [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This bumps the package-json version that npm needs.

@dbolson dbolson marked this pull request as ready for review April 15, 2024 21:15
run: |
./scripts/publish-npm.sh
env:
LD_RELEASE_IS_PRERELEASE: ${{ inputs.prerelease }}
Copy link
Contributor

@sunnyguduru sunnyguduru Apr 15, 2024

Choose a reason for hiding this comment

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

What changes when LD_RELEASE_IS_PRERELEASE is set to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be for tagging the npm package so we don't bump the normal version. We probably won't need to use it.

Copy link
Member

Choose a reason for hiding this comment

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

If you were going to be doing beta/alpha versions, then it would potentially be something you would want. (Or even nightlies 0.7.0-nightly-somesha).

Depending on how you are going to manage releases it may not be relevant.

else
if $LD_RELEASE_IS_PRERELEASE ; then
echo "Publishing with prerelease tag."
npm publish --tag prerelease --provenance --access public || { echo "npm publish failed" >&2; exit 1; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the --provenance flag!

@dbolson dbolson merged commit 461467f into main Apr 16, 2024
2 checks passed
@dbolson dbolson deleted the sc-234872/publish-to-npm branch April 16, 2024 20:41
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.

4 participants