search

jsx-eslint / eslint-plugin-jsx-a11y

Static AST checker for a11y rules on JSX elements.
MIT License
3.38k stars 637 forks source link

Coordinate NodeJS version support with axe-core #968

Closed WilcoFiers closed 8 months ago

WilcoFiers commented 8 months ago

Axe-core product owner here. We are currently planning for axe-core 5.0, and one of the changes we are considering is which version of NodeJS axe-core is compatible with. Axe-core officially support NodeJS >=4, however in practice keeping support for a NodeJS version that old has proven difficult. The reason we still have this is because we want to keep axe-core compatible with jsx-a11y, which is also still on Node 4. This has resulted in a couple issues in the past (https://github.com/dequelabs/axe-core/issues/3537, https://github.com/dequelabs/axe-core/issues/4127).

We want to keep axe-core compatible with jsx-a11y, but in order to do so I think the only options is for jsx-a11y to change what version of NodeJS it supports. Our preference here would be not to support any version of NodeJS that has reached end-of-life. This would be the most secure solution, both for users and our own engineers. If not, we would be open to discuss alternative options too. But what we probably couldn't do is stay on one version indefinitely.

ljharb commented 8 months ago

Can you elaborate on why it's difficult? If CI is testing on it, then it wouldn't be accidentally broken like in the two issues you reference.

WilcoFiers commented 8 months ago

Axe-core's test suite can't run in Node 4. We keep our dependencies up to date to avoid security issues. There are a number of tools we use in testing that were never built to run Node 4. (Think Puppeteer for example). We're adding in smoke tests, which should help, but its far from exhaustive.

The other reason why we're considering this is because we want to reduce axe-core's overall size. Removing the pollyfills makes axe-core smaller. Because axe-core gets loaded into web pages a smaller axe will load faster. It is also a important with Lighthosue, which in the past had issues because of how large axe was getting. Axe-core 5.0 is dropping support for Internet Explorer, which is why those pollyfills were initially introduced.

ljharb commented 8 months ago

You may be underestimating how often modern browsers require polyfills to smooth over edge cases, but understood.

If you did do a breaking change, would you be willing to do backports to v4 for bug and security fixes?

WilcoFiers commented 8 months ago

True enough, and we have plenty of code for that left. But we'd take out the Array.from, Object.entries, Array.prototype.some, etc. pollyfills.

As for security. Yes, we put out security patches on minor versions up at least 18 months old. We've done longer in the past. Its rare we need to though.

ljharb commented 8 months ago

In my experience, semver-major bumps are the thing that imposes the most cost on the ecosystem, so even if I did one here, I wouldn't be committing to following node's (imo needlessly) aggressive EOL timeline - so whatever the new node threshold was, it's what we'd likely have for the next 8 years.

WilcoFiers commented 8 months ago

So if I understand you right you're saying even if jsx-a11y is going to change which version of Node it supports, it would pin to whatever the new one is, and not start following an LTS, or LTS -1 or whatever strategy that would keep it relatively current. Is that right?

If so, how about we consider some kind of compromise. For axe-core the most important thing is to be able to drop the pollyfills. That seems like something jsx-a11y could set up before loading axe-core. Then on our end we can probably just continue to compile down to ES5 syntax. So no async/await, arrow functions, etc. in axe-core. I'll need to double-check that with the team, but this seems to me like something we could do. We can provide an initial list of the necessary pollyfills, and for any future changes your CI tests should tell you if an upgrade required any new pollyfills. Would that be a workable solution?

ljharb commented 8 months ago

Yes, that’s right. Breaking changes should be rare, and committing to doing 1 or 2 a year would be massively harmful to the ecosystem.

Your compromise is workable, but it’d mean we have to violate the decades-old best practice of not mutating the global.

I’m confused why those using axe-core can’t configure their bundler to remove polyfills they don’t need, though (and why are-core needs to bundle prepublish whatsoever).

WilcoFiers commented 8 months ago

Thank you for the feedback. I've had some conversations internally. We've decided to continue supporting whichever version of NodeJS is supported by jsx-a11y. We do not want to create extra work for jsx-a11y, and so if you continue support for NodeJS >=4, then axe-core has to do so as well.

What might still change in axe-core 5.0 is that we'd create a separate version of axe-core that had all the necessary pollyfills for NodeJS >=4. That should be a one-off single line change to adopt. Happy to for us to put in that PR when that time comes.

I think this issue can be closed?

ljharb commented 8 months ago

That makes sense, as long as that didn’t mutate the global to install them ofc.

I appreciate the coordination; things are always better when we work together :-)