-
-
Notifications
You must be signed in to change notification settings - Fork 124
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
fix: setup build generation and rebuild latest dist #81
Conversation
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.
Thanks for looking into this @derberg!
You're right, the built file had an issue. I wrote a bit more about this in the inline comment.
I'm using a workflow to automatically release new versions of the action, therefore build artifacts shouldn't be updated in PRs. Can you revert the change to dist/index.js
? I think with the proposed fix, this should finally work and we can rely that the latest code is in the build artifacts folder 🙂.
package.json
Outdated
@@ -6,7 +6,7 @@ | |||
"scripts": { | |||
"lint": "eslint src", | |||
"test": "jest src", | |||
"build": "rm -rf build && ncc build --minify" | |||
"build": "rm -rf build && ncc build index.js -o dist --minify" |
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 also dug a bit into this. It seems like ncc
has some kind of cache which would result in outdated builds. Even passing no-cache
to the CLI doesn't make a difference.
But I noticed the above script is wrong: The folder for the built assets isn't called "build", but "dist". It seems like once that is fixed, the script works as expected.
"build": "rm -rf build && ncc build index.js -o dist --minify" | |
"build": "rm -rf dist && ncc build" |
@amannn ready for the merge. Lint check failing as fresh dist not pushed |
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.
Thanks a lot @derberg!
🎉 This PR is included in version 3.2.5 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Initial investigation told me your code is perfect. Then I started manual testing with
console.log
and noticed there are no logs showing up in the logs 🤔 it can only mean thatdist
is not reflecting what is on the masterAfter the below change and another build of dist I checked and all works like a charm.
I strongly suggest you configure commit hooks as I suggested in another PR. I did mistakes like this with my actions several times, and then realized that in this case, without a pre-commit hook, it will never stop 😆
Fix: #80