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

Update to Pa11y v7 and other dependency updates #231

Merged
merged 23 commits into from
Mar 11, 2024

Conversation

aarongoldenthal
Copy link
Contributor

@aarongoldenthal aarongoldenthal commented Jan 21, 2024

This PR updates pa11y to 7.0.0, and all other dependencies to the latest supported, applicable versions (not the pure ESM only updates). It also updates the Node versions in the package and GitHub actions.

Dependencies

  • Update async from 2.6.4 to 3.2.5, requiring a minor code change.
  • Update cheerio from 1.0.0-rc.10 to 1.0.0-rc.12.
  • Update commander from 6.2.1 to 11.1.0 (to match pa11y 7.0.0, not the latest), requiring a minor code change.
  • Update globby from 6.1.0 to 11.1.0 (all subsequent versions through 14.0.0 are pure ESM only, but it appears security fixes for 11.x are still being made).
    • Note that 8.0.0 moved to the fast-glob package, which does not have an equivalent nonull capability, so updates were needed. With these changes, a mix of glob and non-glob arguments will only return the glob results (or the non-glob if no results). For example, pa11y-ci **/*.html google.com will only return the glob results (not google.com). All arguments are passed to globby to ensure cases like pa11y-ci **/*.html !foo.html return the correct results.
    • Other packages were investigated, but another viable package with nonull was not found. Even glob, which was the previous engine for globby, has removed it.
      • Package globule does appear to have that option, but hasn't had a commit in a year, so it seemed unmaintained.
    • Also looked at is-glob to check each arg, split the collection, run globby on the globs, and add back the non-globs. One, seems unnecessarily complex. Two, files (like index.html) are considered not globs, so if they matched a glob would produce a duplicate (so, would need to filter out duplicates adding more complexity).
    • This seemed like a reasonable compromise, but if this capability is important, could revert the upgrade. In that case, should probably also add tests to specifically cover that case.
  • Update kleur from 4.1.4 to 4.1.5.
  • Update node-fetch to from 2.6.0 to 2.7.0 (all versions after 2.7.0 are pure ESM only).
    • Node's fetch API is not stable until Node 21 (per the docs). This is only used to fetch sitemaps, but I held off since the LTS releases are still not stable.
  • Update pa11y from 6.2.3 to 7.0.0.
  • Update puppeteer from 9.1.1 to ^20.9.0 (to match pa11y 7.0.0, not the latest, and includes minor updates)
    • Note that puppeteer now outputs to stderr a warning about headless: true soon using new as the default. Several tests used the console output, which is a combination of both stdout and stderr, so they were updated to check stdout only.
    • After this update, was seeing some ERROR: The process "xxxx" not found. messages in stderr, so changed process.exit() calls to setting process.exitCode to allow a graceful exit here.

Dev Dependencies

  • Update eslint from 7.27.0 to 8.56.0.
  • Update mocha from 8.4.0 to 10.3.0.
  • Update sinon from 11.1.0 to 17.0.1.

Other changes

  • Update engines:node to >=18 (matching pa11y 7.0.0).
  • Update actions to latest Node versions (matching pa11y 7.0.0) and unpinned Ubuntu version.
  • Added keywords to package.json to help with discoverability (was empty, now matches pa11y).
  • Refreshed lockfile to update all transitive dependencies.

Notes

  • protocolify was not updated since all updates are pure ESM only.
  • proclaim and some of its dependencies are deprecated, but it seemed like broader test updates would make more sense as part of the effort to move pa11y-ci functionality into pa11y, which already moved to jest.
  • The MacOS actions have had intermittent and inconsistent test failures, but thought that might be easier to isolate by someone running locally on a Mac. I also appear to not have permissions to rerun in the parent repo, although in my fork rerunning produced a successful run.

Will close (once released) #198

@danyalaytekin danyalaytekin added this to the 4.0.0 milestone Mar 10, 2024
Copy link
Member

@danyalaytekin danyalaytekin left a comment

Choose a reason for hiding this comment

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

This is fantastic, thank you @aarongoldenthal, and also for the care with the queue's signature change (I had to read up on that), process output assertion, and the rest. I've made a couple of minor changes, including an attempt to fix the flaky tests by increasing the timeout, similar to a change made for Pa11y:

Re: globbing

The change to the globbing behaviour does seem significant and could surprise users. I'd favour reverting globby to the final version of ^7, or perhaps even ^6 for now, since 7 seems to come with its own significant change, which I haven't thought much about, but an npm audit on 6.1.1 has produced no warning locally, and the tests pass. If I've missed a reason why an update is definitely required, though, please correct me. cc @jpw @josebolos @hollsk in case you feel differently and are happy with the change.

In any case, at some point (not here!) we should probably add a couple more tests, just to clarify the globbing behaviours we do support.

bin/pa11y-ci.js Outdated Show resolved Hide resolved
@aarongoldenthal
Copy link
Contributor Author

Thanks @danyalaytekin. I remembered you had looked at a similar flaky test on Mac issues before, but couldn't remember exactly where that was.

I reverted globby back to 6.1.0 to avoid the behavioral change, but I left the description so those details weren't lost. I don't know of a driver other than trying to update where possible to keep current.

@danyalaytekin danyalaytekin merged commit 79f3449 into pa11y:main Mar 11, 2024
7 checks passed
stevenleija added a commit to stevenleija/pa11y-ci-stevenleija that referenced this pull request May 2, 2024
* origin/master:
  Bump ip from 1.1.8 to 1.1.9
  Update to Pa11y v7 and other dependency updates (pa11y#231)
  Version `3.1.0` (pa11y#226)
  Add JavaScript config example to README (pa11y#197)
  Bump @babel/traverse from 7.16.0 to 7.23.3 (pa11y#222)
  Bump semver from 6.3.0 to 6.3.1 (pa11y#221)
  Bump json5 from 2.2.0 to 2.2.3 (pa11y#219)
  Bump ansi-regex from 3.0.0 to 3.0.1 (pa11y#218)
  Bump debug from 4.1.1 to 4.3.1 (pa11y#216)
  Bump node-fetch from 2.6.1 to 2.6.7 (pa11y#217)
  Upgrade to `pa11y@6.2.3`, add publishing workflow (pa11y#213)
  Update integration tests to support all platforms, including Windows (pa11y#177)

# Conflicts:
#	bin/pa11y-ci.js
#	package-lock.json
#	package.json
@aarongoldenthal aarongoldenthal deleted the dependency-updates branch May 27, 2024 14:28
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.

2 participants