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

Use Pa11y 7, require Node 18, fix issues #149

Merged
merged 53 commits into from
Apr 8, 2024
Merged

Use Pa11y 7, require Node 18, fix issues #149

merged 53 commits into from
Apr 8, 2024

Conversation

danyalaytekin
Copy link
Member

@danyalaytekin danyalaytekin commented Nov 7, 2023

This PR updates the service to use pa11y@7, updates the minimum version of Node.js required to 18, and makes several other changes to modernise the project.

Changes

  • Dependencies
    • Raise minimum Node.js requirement to 18 from 12
    • Use pa11y@7 in place of 6
    • Use hapi@21 in place of 20
    • Use actions@setup-node@4 in place of 3
    • Remove request
  • Code
    • Restore CI build's Ubuntu environment to ubuntu-latest from ubuntu-20.04
    • Allow ES2020 syntax, and use it in some places to reduce repetition & tighten signatures
    • Replace remaining use of make with npm scripts
    • Use util.promisify to flatten some async code
    • Replace use of request with fetch throughout
    • Rename some ambiguous use of env to mode for clarity
    • Remove some comments where they repeated adjacent code names
    • Rewrite several tests to async syntax, replacing done syntax
    • Tidy up test setup
  • Documentation
    • Document an option to use nektos/act to test locally

Changes during review process

  • d8d977e Add link to Docker Desktop
  • d24182d Add link to nektosact.com and mention other ways to install act
  • 0f9b6e0 Be clearer that act is for locally testing GitHub Actions
  • d857fb1 Mention installation options across both Docker Desktop and act; use shortlinks for consistency

Thanks for the review and documentation fixes @hollsk

@danyalaytekin danyalaytekin added this to the 5.0 milestone Nov 7, 2023
@danyalaytekin danyalaytekin self-assigned this Nov 7, 2023
@danyalaytekin danyalaytekin force-pushed the use-pa11y-7 branch 2 times, most recently from 7f471e0 to 4a33294 Compare November 12, 2023 22:06
… it's more relevant; fix two inconsistencies in the app
@danyalaytekin danyalaytekin added the status: work required Work in progress label Mar 13, 2024
@danyalaytekin danyalaytekin changed the title Use Pa11y 7, require Node 18, fix issues [WIP, draft] Use Pa11y 7, require Node 18, fix issues Mar 13, 2024
@danyalaytekin danyalaytekin added type: enhancement type: maintenance dependencies Pull requests that update a dependency file and removed status: work required Work in progress labels Mar 17, 2024
@danyalaytekin danyalaytekin marked this pull request as ready for review March 17, 2024 23:36
@danyalaytekin danyalaytekin changed the title [WIP, draft] Use Pa11y 7, require Node 18, fix issues Use Pa11y 7, require Node 18, fix issues Mar 17, 2024
Copy link
Member

@hollsk hollsk left a comment

Choose a reason for hiding this comment

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

Superb, thank you so much @danyalaytekin 🙏

There's a few lint warnings in model/task.js. All of them are about complexity though, and i think it's probably more important at this point to just get this merged and worry about decomplexifying those blocks later.

Aside from that, i just added suggestions with direct links to Docker and Act.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@danyalaytekin
Copy link
Member Author

danyalaytekin commented Apr 8, 2024

Thanks @hollsk, I've committed those and added one more (added a list at the bottom of the PR description). I don't think the linters' warnings were introduced by this PR, including the nesting one since the previous version had nesting 3 as well, unless the rule doesn't apply to inline functions, but I agree about placating them at some point for the various codebases they're affecting.

@danyalaytekin danyalaytekin requested a review from hollsk April 8, 2024 05:18
@hollsk
Copy link
Member

hollsk commented Apr 8, 2024

Oh yes, love opening up a codebase and finding ancient lint errors in it 😆 Looks perfect, thank you!

@danyalaytekin danyalaytekin merged commit 21b7472 into main Apr 8, 2024
10 checks passed
@danyalaytekin danyalaytekin deleted the use-pa11y-7 branch April 8, 2024 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file type: enhancement type: maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants