Skip to content

Conversation

@Isha-Sharma
Copy link
Collaborator

@Isha-Sharma Isha-Sharma commented Nov 28, 2025

Description

Upload package to github

Resolutions

  • fixes getfundwave/discussions#690

Copilot AI review requested due to automatic review settings November 28, 2025 08:32
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR migrates the package publishing infrastructure from GitLab to GitHub. The changes update repository URLs, reorganize build scripts, and reconfigure the CI/CD workflow to publish packages to GitHub Packages instead of GitLab.

Key changes:

  • Updated repository URLs from getfundwave to fundwave organization
  • Refactored build scripts to separate CJS and ESM builds
  • Modified CI/CD workflow to publish to GitHub Packages with proper authentication

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
package.json Updated repository URLs to fundwave org and reorganized build scripts
.github/workflows/npm-publish.yml Reconfigured workflow to publish to GitHub Packages instead of GitLab

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

"build": "tsc --module commonjs --outDir ./dist/cjs",
"prepare": "npm run build && npm run build:esm",
"build:cjs": "tsc --module commonjs --outDir ./dist/cjs",
"build": "npm run build:cjs && npm run build:esm",
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The removal of the 'prepare' script may cause issues for package consumers. The 'prepare' script automatically runs during 'npm install' and ensures the package is built before use. Without it, developers installing this package from git or using 'npm link' won't have built artifacts available. Consider keeping the 'prepare' script or documenting this breaking change.

Suggested change
"build": "npm run build:cjs && npm run build:esm",
"build": "npm run build:cjs && npm run build:esm",
"prepare": "npm run build",

Copilot uses AI. Check for mistakes.
Comment on lines +61 to +65
- name: Setup .npmrc
run: |
echo "@fundwave:registry=https://npm.pkg.github.com" >> ~/.npmrc
echo "//npm.pkg.github.com/:_authToken=${{ secrets.GITHUB_TOKEN }}" >> ~/.npmrc
echo "ignore-scripts=true" >> ~/.npmrc
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The .npmrc configuration is set up before 'npm ci' but the package still needs to be published. The 'ignore-scripts=true' setting will prevent lifecycle scripts from running during install, but this configuration will also affect the 'npm publish' command later. Consider moving this setup to just before the publish step, or use a more targeted approach like creating a separate .npmrc for publishing.

Copilot uses AI. Check for mistakes.
GITLAB_TOKEN: ${{secrets.GITLAB_TOKEN}}
- name: Build package
run: npm run build

Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The publish command relies on the .npmrc configured earlier, but there's no explicit '--registry' flag or scope verification. Since the package scope is '@fundwave', ensure the package.json includes '"name": "@fundwave/fetch-queue"' to match the registry configuration. Without proper scoping, the publish may fail or publish to the wrong registry.

Suggested change
- name: Verify package name scope
run: |
PKG_NAME=$(jq -r .name package.json)
if [ "$PKG_NAME" != "@fundwave/fetch-queue" ]; then
echo "Error: package.json name must be '@fundwave/fetch-queue', found '$PKG_NAME'"
exit 1
fi

Copilot uses AI. Check for mistakes.
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.

2 participants