-
Notifications
You must be signed in to change notification settings - Fork 63
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 pa11y-webservice to use pa11y 5.x #58
Conversation
Thanks @wilco42, this is great. I'm gonna need a little time to review properly and test but appreciate the work you've put in :) |
We've just got this branch running at Springer Nature and it's looking great! 👍 We did have to make one change in Somebody else had the same issue on StackOverflow recently, so I think this is probably going to be fairly common and something that we should be allowing people to pass in. It shouldn't be the default option. I think it should be a global setting, not a task-specific setting - if one task needs it, they probably all will, right? As it's related to the environment that Chrome is running in. Does it seem right to add it as an option in the config object for webservice? |
@rowanmanning any chance of getting this PR reviewed? I am looking to work also on updating dependencies and making this run within Chrome... |
Adds semicolons after handlers
Looking at Travis, we have a slight problem. Pa11y 5 hikes the minimum Node version to 8. Our build jobs are for 4, 5, & 6. |
model/task.js
Outdated
timeout: (task.timeout || 30000), | ||
wait: (task.wait || 0), | ||
ignore: task.ignore, | ||
actions: task.actions || [], | ||
phantom: {}, | ||
chromeLaunchConfig: { | ||
args: ['--no-sandbox'] |
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.
--no-sandbox
shouldn't be the default option. The Chromium team specifically warn against using it, so the fact that we're using it in the first place to get it running is the wrong thing to do 😆 so it'd be doubly-triply wrong to force it on all users, some of whom may actually have more appropriate kernel setups.
I have made updates on Node.js versions in my PR #68, which migrates to hapi v17, and which I would continue to work on further updates but since I have not seen any interest from the maintainers, I am not sure how to proceed. |
Hey @paazmaya - we're grateful for the work you've put in so far! We're a small team who work on Pa11y outside of our day jobs, and you've just caught us at a really busy/bad time. We're working to get back on top of things, thanks for your patience! 😄 |
This commit drops versions 4, 5 and 6 from the travis config, and replace them with versions 8, 10 and 12. We broadly follow the [Springer Nature Open Source support guidelines](https://github.com/springernature/frontend-playbook/blob/14ea315e877a8941cc995c6433250a793cffa85f/practices/open-source-support.md#node-versions) with regards to the Node versions that we support. We aim to support every version that the Node foundation offers [Long-term support (LTS)](https://github.com/nodejs/Release) for, so at the time of writing this is versions 8 and 10, plus version 12 which will become the next LTS.
Update the travis config to test on node 8/10/12
Implements #56
A couple of notes: