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

automate npm publish #1427

Merged
merged 6 commits into from
Jun 8, 2021
Merged

automate npm publish #1427

merged 6 commits into from
Jun 8, 2021

Conversation

yen-tt
Copy link
Contributor

@yen-tt yen-tt commented Jun 7, 2021

GH workflow action which, on successful test and build steps on master branch, will publish to npm registry

J=SLAP-1353
TEST=manual

created mock repo and npm package, made a new tagged release and verified it automatically publish to npm test package.

GH workflow action which, on successful test and build steps on master branch, will publish to npm registry

J=SLAP-1353
TEST=manual

created mock repo and npm package, made a new tagged release and verified it automatically publish to npm test package.
@coveralls
Copy link

coveralls commented Jun 7, 2021

Coverage Status

Coverage remained the same at 56.793% when pulling 835259a on dev/automate-npm-publish into deaa78d on develop.

with:
node-version: ${{ matrix.node-version }}
- run: npm ci
- run: npm run test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to include the test here? If the tests fail will the publish fail? There shouldn't be code in master that fails our tests.

Copy link
Contributor Author

@yen-tt yen-tt Jun 7, 2021

Choose a reason for hiding this comment

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

ah I added it in as a safety check, but if we already verify that before pushing to master then I guess this step is not necessary.

node-version: ${{ matrix.node-version }}
registry-url: https://registry.npmjs.org/
- run: npm ci
- run: npm run build
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for my edification, we're not including the contents of src or test in what we publish to NPM, right? We're essentially just publishing the dist directory and some manifest files (package.json, etc.)

Copy link
Member

Choose a reason for hiding this comment

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

Currently we include everything except the things specified in .npmignore. In answers-core, we instead use the 'files' param in package.json to specify which files and folders to include. I think we should switch answers-search-ui to do that as well. I think the dist folder is the only one we need

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 made the change in package.json to include dist/ folder only (npm will also automatically include package.json, license, and readme file when publish)

Copy link
Collaborator

Choose a reason for hiding this comment

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

With the files addition to the package.json do we still need .npmignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Would need it if we want to exclude something specific from files, but since node_modules is automatically ignored and DS_Store is never going to be push to repo, I will remove .npmignore

@yen-tt yen-tt merged commit 49b6d0f into develop Jun 8, 2021
@yen-tt yen-tt deleted the dev/automate-npm-publish branch June 8, 2021 20:10
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.

None yet

4 participants