-
Notifications
You must be signed in to change notification settings - Fork 783
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: add grunt build step to npm run release #844
Conversation
@@ -56,7 +56,7 @@ | |||
"version": "echo \"use 'npm run release' to bump axe-core version\" && exit 1", | |||
"prepublishOnly": "grunt build", | |||
"postinstall": "node build/utils/postinstall.js", | |||
"release": "standard-version && node build/sri-update" |
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.
Maybe make grunt build
a prerelease step instead?
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.
There isn't an npm prerelease
script, so we'd have to make one...but it would only have one thing in it. We do also call grunt build
on prepublishOnly
so we don't publish without building first. But honestly I don't see what a custom script would gain us since this is such a simple change.
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.
The benefit of using a prerelease here is it's much easier to edit/read since we've already got several commands here.
If you add a pre
before a custom script, npm will respect it.
With a package.json that looks like:
"scripts": {
"prerelease": "echo prerelease",
"release": "echo release"
},
Running npm run release
will output both prerelease and release:
∴ npm run release
> foo@1.0.0 prerelease /Users/stephen/foo
> echo prerelease
prerelease
> foo@1.0.0 release /Users/stephen/foo
> echo release
release
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'm still not really seeing what this will gain us. It also doesn't address prepublishOnly
, which isn't the same context as release
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.
The only reason I'm suggesting this (see previous comment) is that it'll make the release
script easier to read/edit.
Really not a big deal tho
package.json
Outdated
@@ -56,7 +56,7 @@ | |||
"version": "echo \"use 'npm run release' to bump axe-core version\" && exit 1", | |||
"prepublishOnly": "grunt build", | |||
"postinstall": "node build/utils/postinstall.js", | |||
"release": "standard-version && node build/sri-update" | |||
"release": "grunt build && standard-version && node build/sri-update" |
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 don't think this solves the problem. Here's the order things need to run in:
- package.json is bumped
- axe.js / axe.min.js are build
- node build/sri-update is run
- git commit is run
This is creating a commit with the wrong version of axe-core in axe.js, and the sri-history isn't updated until after you've committed and pushed the tag. I don't think this can be solved without a lifecycle hook:
https://github.com/conventional-changelog/standard-version#lifecycle-scripts
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.
Ok, I'll look into that. Thanks for the info
48eed64
to
9e2b8df
Compare
9e2b8df
to
55d3a10
Compare
Ok it took some wrangling, but I think I got it! SRI history is now being updated and committed as part of the |
* fix(core): Explicitly name the axe module 'axe-core' Avoid the "Mismatched anonymous define() modules" error when the axe script is injected in a page that uses requireJS Closes #849 * fix: Prevent color rules from crashing Chrome 66+ #856 (#861) * chore: Release axe-core 3.0.2 * chore: Enable Greenkeeper for managing dependencies (#847) * chore: add Greenkeeper config file * chore(package): update dependencies * chore(package): update dependencies * chore(package): update dependencies * chore(package): update dependencies * chore(package): update dependencies * chore(package): update dependencies * docs(readme): add Greenkeeper badge * chore: update to use babel-core * chore: update to latest uglify config properties `bracketize` became `braces` and `expect` became `reserved` * chore: add sri-history lifecycle hook to release (#844) * chore: disable growl to prevent errors in testing * chore: Rename Jest example to help greenkeeper (#871) * chore: rename jest example to help greenkeeper plus signs are invalid in filenames/directories Closes #869 * chore: add config to jest_react example Closes #865 * fix(core): Explicitly name the axe module 'axe-core' Avoid the "Mismatched anonymous define() modules" error when the axe script is injected in a page that uses requireJS Closes #849 * test(core): Validate that the axe module is named 'axe-core' Added integration test to check the value of the first argument to define() Closes #849
This should hopefully get axe-core into the proper state before creating a new entry for sri-history. My best guess as to why sri-history was changing was because it had been updated without running
grunt build
first, so the source was actually different. This makes it an automatic build step instead of relying on the publisher to remember it.