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: colorio.js patch mocking CSS #4456
base: develop
Are you sure you want to change the base?
Conversation
Bumps the npm-low-risk group with 9 updates in the / directory: | Package | From | To | | --- | --- | --- | | [@babel/core](https://github.com/babel/babel/tree/HEAD/packages/babel-core) | `7.24.4` | `7.24.5` | | [@babel/preset-env](https://github.com/babel/babel/tree/HEAD/packages/babel-preset-env) | `7.24.4` | `7.24.5` | | [@babel/runtime-corejs3](https://github.com/babel/babel/tree/HEAD/packages/babel-runtime-corejs3) | `7.24.4` | `7.24.5` | | [clean-jsdoc-theme](https://github.com/ankitskvmdam/clean-jsdoc-theme) | `4.2.18` | `4.3.0` | | [colorjs.io](https://github.com/LeaVerou/color.js) | `0.4.3` | `0.5.0` | | [core-js](https://github.com/zloirock/core-js/tree/HEAD/packages/core-js) | `3.36.1` | `3.37.0` | | [jsdoc](https://github.com/jsdoc/jsdoc) | `4.0.2` | `4.0.3` | | [selenium-webdriver](https://github.com/SeleniumHQ/selenium) | `4.19.0` | `4.20.0` | | [typescript](https://github.com/Microsoft/TypeScript) | `5.4.4` | `5.4.5` | Updates `@babel/core` from 7.24.4 to 7.24.5 - [Release notes](https://github.com/babel/babel/releases) - [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md) - [Commits](https://github.com/babel/babel/commits/v7.24.5/packages/babel-core) Updates `@babel/preset-env` from 7.24.4 to 7.24.5 - [Release notes](https://github.com/babel/babel/releases) - [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md) - [Commits](https://github.com/babel/babel/commits/v7.24.5/packages/babel-preset-env) Updates `@babel/runtime-corejs3` from 7.24.4 to 7.24.5 - [Release notes](https://github.com/babel/babel/releases) - [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md) - [Commits](https://github.com/babel/babel/commits/v7.24.5/packages/babel-runtime-corejs3) Updates `clean-jsdoc-theme` from 4.2.18 to 4.3.0 - [Release notes](https://github.com/ankitskvmdam/clean-jsdoc-theme/releases) - [Changelog](https://github.com/ankitskvmdam/clean-jsdoc-theme/blob/master/CHANGELOG.md) - [Commits](https://github.com/ankitskvmdam/clean-jsdoc-theme/commits) Updates `colorjs.io` from 0.4.3 to 0.5.0 - [Release notes](https://github.com/LeaVerou/color.js/releases) - [Commits](color-js/color.js@v0.4.3...v0.5.0) Updates `core-js` from 3.36.1 to 3.37.0 - [Release notes](https://github.com/zloirock/core-js/releases) - [Changelog](https://github.com/zloirock/core-js/blob/master/CHANGELOG.md) - [Commits](https://github.com/zloirock/core-js/commits/v3.37.0/packages/core-js) Updates `jsdoc` from 4.0.2 to 4.0.3 - [Release notes](https://github.com/jsdoc/jsdoc/releases) - [Changelog](https://github.com/jsdoc/jsdoc/blob/4.0.3/CHANGES.md) - [Commits](jsdoc/jsdoc@4.0.2...4.0.3) Updates `selenium-webdriver` from 4.19.0 to 4.20.0 - [Release notes](https://github.com/SeleniumHQ/selenium/releases) - [Commits](SeleniumHQ/selenium@selenium-4.19.0...selenium-4.20.0) Updates `typescript` from 5.4.4 to 5.4.5 - [Release notes](https://github.com/Microsoft/TypeScript/releases) - [Changelog](https://github.com/microsoft/TypeScript/blob/main/azure-pipelines.release.yml) - [Commits](microsoft/TypeScript@v5.4.4...v5.4.5) --- updated-dependencies: - dependency-name: "@babel/core" dependency-type: direct:development update-type: version-update:semver-patch dependency-group: npm-low-risk - dependency-name: "@babel/preset-env" dependency-type: direct:development update-type: version-update:semver-patch dependency-group: npm-low-risk - dependency-name: "@babel/runtime-corejs3" dependency-type: direct:development update-type: version-update:semver-patch dependency-group: npm-low-risk - dependency-name: clean-jsdoc-theme dependency-type: direct:development update-type: version-update:semver-minor dependency-group: npm-low-risk - dependency-name: colorjs.io dependency-type: direct:development update-type: version-update:semver-minor dependency-group: npm-low-risk - dependency-name: core-js dependency-type: direct:development update-type: version-update:semver-minor dependency-group: npm-low-risk - dependency-name: jsdoc dependency-type: direct:development update-type: version-update:semver-patch dependency-group: npm-low-risk - dependency-name: selenium-webdriver dependency-type: direct:development update-type: version-update:semver-minor dependency-group: npm-low-risk - dependency-name: typescript dependency-type: direct:development update-type: version-update:semver-patch dependency-group: npm-low-risk ... Signed-off-by: dependabot[bot] <[email protected]>
package.json
Outdated
@@ -113,7 +113,8 @@ | |||
"prepare": "husky", | |||
"prebuild": "node ./build/check-node-version.js", | |||
"pretest": "node ./build/check-node-version.js", | |||
"postbuild": "prettier --write ./locales/_template.json ./doc/rule-descriptions.md" | |||
"postbuild": "prettier --write ./locales/_template.json ./doc/rule-descriptions.md", | |||
"postinstall": "patch-package" |
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.
We really want to avoid adding a postinstall script to axe-core - this would add it as a step that axe-core consumers run when doing npm install axe-core
, which we don't want. Instead, we'll want to run the command line as a build step. This will likely mean adding a grunt task wrapping it to /build/tasks/
that you reference from Gruntfile.js
.
patches/colorjs.io+0.5.0.patch
Outdated
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.
This patch is way bigger than it needs to be; we don't need to patch the sourcemap files and we don't need to patch all of their dist variants, just the one we actually use in our build process (I'm not sure offhand which one that is, but the test should tell you when you've found it)
patches/color.unpatched.js
Outdated
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 we should check this into the repo; if we need to test against this, we should emit it by copying from node_modules as part of the build step that patches the original
patches/colorjs.io+0.4.5.patch
Outdated
- if (CSS.supports("color", str)) { | ||
+ if (CSS?.supports("color", str)) { |
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.
This one shouldn't need to be patched since by here CSS.supports
exists and is not null
due to the if statement above.
patches/colorjs.io+0.4.5.patch
Outdated
ret = new String(ret); | ||
ret.color = color; | ||
} | ||
diff --git a/node_modules/colorjs.io/dist/color.js b/node_modules/colorjs.io/dist/color.js |
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.
We should only need to patch one of these files (either color.js
or color.cjs
)
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.
colorio.js
's package.json
has the following exports
defined:
"exports": {
".": {
"types": "./types/index.d.ts",
"import": "./dist/color.js",
"require": "./dist/color.cjs"
and also in the package json
"main": "./dist/color.cjs",
"module": "./dist/color.js",
so it seemed safer to do both, but we can pick one if we're certain only that one will be used
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.
Our build process will only use one of them to build, so in favor of making the patch as small possible I think we should patch the one that our build grabs and leave the other alone.
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.
Sounds good, switched to only color.js
then
I'll need to repatch it when #4449 is merged
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.
Nevermind, I rebased my branch so now it'll flow through naturally in theory. Switched patch to be for colorjs.io 0.4.3
instead.
b830e6a
to
ac45ddf
Compare
Gruntfile.js
Outdated
@@ -6,12 +6,14 @@ camelcase: ["error", {"properties": "never"}] | |||
module.exports = function (grunt) { | |||
'use strict'; | |||
|
|||
grunt.loadNpmTasks('grunt-exec'); |
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.
At some point, we're going to have to stop adding tasks to Grunt if we're ever going to get rid of it.
Should now be that time?
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.
Would be happy too, if we know what we'd rather do instead. @WilcoFiers suggested this should be in the build step. Thus there are multiple ways we could do this:
- As-is in the grunt build step
- Another custom node script like we do for several other scripts referenced in the
package.json
- Tacked onto the build command in
package.json
with&& cp path... && npx patch-package name...
etc - A different build tool, but that blows up the scope of this task
I'm of the opinion that build tools are changing all the time and it makes it feel strange to suggest just switching to the new hotness, but that just writing more custom node scripts in /build/*
results in us reinventing the wheel. Still, moving towards vite/esbuild etc in the future may be my minor preference.
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.
Since we want this to be part of the build it needs to go in the grunt build
step so npm develop
builds it as well. We won't be moving off grunt any time soon and not in this PR so a grunt step is fine for now.
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 I said it needed to be in grunt :P. We did a good while back agree not to add more grunt tasks if we could at all avoid it. It seems viable to put this work in prebuild / predevelop scripts instead. Did you try that @gaiety-deque? If that was more difficult I can live with a Grunt solution, but if we can avoid these extra dependencies + more grunt tasks I think that's the way to go on this.
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.
Apologies @WilcoFiers I didn't mean to imply that you explicitly said it had to be in the grunt build step, just that the issue requested it be a part of the build step. Since our build step just calls "grunt" currently and the pre/post build steps were more test-like in nature it felt like we were intentionally doing these things in grunt.
Made a change 1785f2c to move it to be commands run in the package json scripts.
This comment was marked as spam.
This comment was marked as spam.
.eslintignore
Outdated
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.
This file was pulled in ESLint 9.
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.
Weird thought I fixed this but must have snuck back in through all the rebasing, fixed 87c7845
Gruntfile.js
Outdated
@@ -6,12 +6,14 @@ camelcase: ["error", {"properties": "never"}] | |||
module.exports = function (grunt) { | |||
'use strict'; | |||
|
|||
grunt.loadNpmTasks('grunt-exec'); |
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 I said it needed to be in grunt :P. We did a good while back agree not to add more grunt tasks if we could at all avoid it. It seems viable to put this work in prebuild / predevelop scripts instead. Did you try that @gaiety-deque? If that was more difficult I can live with a Grunt solution, but if we can avoid these extra dependencies + more grunt tasks I think that's the way to go on this.
wilco and I believe marque is to blame because it is animated This fixes the `all-rules` check, particularly on Firefox. We believe the issue is because marque has a moving bounding client rect, leading to inconsistent results. So instead we set the motion to none for a consistent testing environment. --- To see that it works, I've run the CI tests on this branch five times. It only failed on one unrelated flakey test this does not address. Also, #4456 finally builds after many consistent test failures in the same place in a row after rebasing on this change. We can see this test failing on other branches, such as `develop` in places like this: https://app.circleci.com/pipelines/github/dequelabs/axe-core/6332/workflows/b3e2a293-c89b-4201-898d-1c6c64ee2764 which shows it has nothing to do with the other PR.
"prebuild": "node ./build/check-node-version.js && npm run unpatch", | ||
"predevelop": "npm run unpatch", |
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.
"prebuild": "node ./build/check-node-version.js && npm run unpatch", | |
"predevelop": "npm run unpatch", | |
"prebuild": "node ./build/check-node-version.js && npm run patch", | |
"predevelop": "npm run patch", |
Object.defineProperty(window, 'CSS', { value: null }); | ||
} | ||
|
||
describe('patch test', function () { |
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.
It seems this is missing a test. This should have failed because you're running unpatch before build. Lets confirm we actually applied the patch into axe-core by importing both axe.js and axe.min.js with window.CSS = null.
"pretest": "node ./build/check-node-version.js", | ||
"postbuild": "prettier --write ./locales/_template.json ./doc/rule-descriptions.md" | ||
"postbuild": "prettier --write ./locales/_template.json ./doc/rule-descriptions.md && npm run patch", | ||
"postdevelop": "npm run patch" |
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.
This is never going to run. npm run develop
watches for changes. It never completes unless you terminate the process. And if you do that, postdevelop doesn't run.
"pretest": "node ./build/check-node-version.js", | ||
"postbuild": "prettier --write ./locales/_template.json ./doc/rule-descriptions.md" | ||
"postbuild": "prettier --write ./locales/_template.json ./doc/rule-descriptions.md && npm run patch", |
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 suspect you mean to unpatch here. I'm not sure about this unpatching at the end of the flow. It feels like there are potential gaps there, especially with develop. Might be better if we had one script that unpatches, copies, then patches, which runs before develop and build. That feels like a slightly safer way to ensure the files in patch/unpatched
are always up to date.
@@ -111,9 +113,11 @@ | |||
"sri-validate": "node build/sri-update --validate", | |||
"fmt": "prettier --write .", | |||
"prepare": "husky", | |||
"prebuild": "node ./build/check-node-version.js", | |||
"prebuild": "node ./build/check-node-version.js && npm run unpatch", |
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.
One more thing to consider is that we could use the prepare
script instead of a prebuild script. The reason we can't use postinstall is because that would trigger when someone uses npm i axe-core
, but prepare
only run when you call npm install in the axe-core repo. It would prevent having to run patch / unpatch over and over. @straker @dbjorge, do you see any reason not to do that?
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 main gotchas I'm aware of with prepare
are that:
- It runs within a package consumer if the consumer uses a
"git:github.com/dequelabs/axe-core.git"
dependency on axe-core rather than a more typical npm dependency - It's not treated consistently by all packaging tools; yarn v2+ ignores it, for example.
I don't think either of those matter enough to make me want to avoid it here; the former case is already broken if it isn't running our build process, and I don't think we need optimize for the latter (so long as there's a test that would catch the patch not applying as expected).
Adds
patch-package
as suggested in the issue this PR fixes. The goal is to allowwindow.CSS
to be mockednull
in situations such as using JSdom.Tests cover a version that is patched and one that is unpatched to demonstrate the patch truly fixes the issue.
fixes: #4400
Developer Notes/Questions for review:
postinstall
, what should it be instead?patches/color.unpatched.js
but perhaps there's a better waydist
files