-
Notifications
You must be signed in to change notification settings - Fork 16
feat(plugin-axe): implement core plugin functionality #1141
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
base: main
Are you sure you want to change the base?
Conversation
|
View your CI Pipeline Execution ↗ for commit 0560b3e
☁️ Nx Cloud last updated this comment at |
@code-pushup/ci
@code-pushup/cli
@code-pushup/core
@code-pushup/create-cli
@code-pushup/models
@code-pushup/nx-plugin
@code-pushup/axe-plugin
@code-pushup/coverage-plugin
@code-pushup/eslint-plugin
@code-pushup/js-packages-plugin
@code-pushup/jsdocs-plugin
@code-pushup/lighthouse-plugin
@code-pushup/typescript-plugin
@code-pushup/utils
@code-pushup/models-transformers
commit: |
3680cce to
e4e390f
Compare
e4e390f to
72fc042
Compare
feat(plugin-axe): implement runner with Playwright/Axe integration
c8c6dd5 to
972b3f1
Compare
972b3f1 to
6d66b16
Compare
fa31898 to
5dcf3ad
Compare
Code PushUp🤨 Code PushUp report has both improvements and regressions – compared current commit 517cd6a with previous commit 4a982a5. 🕵️ See full comparison in Code PushUp portal 🔍 🏷️ Categories👍 2 groups improved, 👎 2 groups regressed, 👍 4 audits improved, 👎 6 audits regressed, 15 audits changed without impacting score🗃️ Groups
17 other groups are unchanged. 🛡️ Audits
585 other audits are unchanged. |
597803d to
f1fc866
Compare
packages/plugin-axe/project.json
Outdated
| "build": { | ||
| "dependsOn": ["^build", "install-browser"] | ||
| }, |
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 like that the browser installation is now part of our Nx graph. 👍
However, I'm wondering if build is the right target where we should define the dependency. We don't need have the browser installed in order to build the package. So in some scenarios (e.g., checking for type errors), we may be adding an unnecessary and expensive operation.
As I understand, the browser installation is only required for our code-pushup-axe target (you've already defined this dependency in our root project.json) and for e2e-test target of plugin-axe-e2e. So I'm thinking it would be more optimal to add ^install-browser to the dependsOn array of the e2e-test target instead. This could even be applied globally for all e2e-test targets 🤔
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 tried swapping configurations around and believe the target was unnecessary after all.
Now that the plugin handles Chromium installation, Playwright's own caching mechanism will prevent repeated downloads. Playwright caches browsers in the user's cache directory, which means that locally, the browser is only downloaded once on first use. In CI, the browser is downloaded fresh on each workflow run, which is Playwright's recommended approach.
d9630ff to
5a9d83a
Compare
5a9d83a to
0560b3e
Compare
Closes #1134
Portal preview