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

RFC: option to disable polyfills #4087

Closed
cdaringe opened this issue Jul 11, 2023 · 5 comments
Closed

RFC: option to disable polyfills #4087

cdaringe opened this issue Jul 11, 2023 · 5 comments
Labels
feat New feature or enhancement

Comments

@cdaringe
Copy link

Product

axe-core

Feature Description

Problem

Polyfills are currently forced.

In my env, I do not want polyfills, because the break compatibility with my environment (see capricorn86/happy-dom#978). I ended up doing my own polyfill to deactivate axe's polyfill, but it's worth consideration.

Discussion

It would be great to add a flag to bypass the polyfill logic and allow me to get me my environment runtime into top shap!

@cdaringe cdaringe added feat New feature or enhancement ungroomed Ticket needs a maintainer to prioritize and label labels Jul 11, 2023
@straker
Copy link
Contributor

straker commented Jul 12, 2023

Thanks for the issue. Could you help me understand a bit how the polyfill affects your code? I took a quick look through the code and saw that the Node file defines an isConnected property. If an instance of that class is the same as the one on window.Node, then I would assume the guard we have around our polyfill would prevent the polyfill from activating. If the Node class isn't the one on window then I'm not sure how adding an isConnected property to Node then affects your custom classes.

@straker straker removed the ungroomed Ticket needs a maintainer to prioritize and label label Jul 12, 2023
@cdaringe
Copy link
Author

The field is present on instantiation, but not on the prototype. The polyfill checks the prototype, which im unclear is correct or incorrect behavior. MDN says that the field should exist on Node. The spec doesnt seem to suggest that prototype specially is guaranteed to have the field https://dom.spec.whatwg.org/#node, or that it’s even statically available. Seems like some impla do use prototypical inheritance, but im not clear if its required (appears not). My gut says the polyfill detection may be too presumptive. Tldr, happy-dom doesnt set the field on the prototype, that seems OK, but axe then mutates a getter on happy-dom and breaks happy-dom

@cdaringe
Copy link
Author

Typing from Phone, sorry for the brevity or terseness

@dbjorge
Copy link
Contributor

dbjorge commented Jul 13, 2023

Thanks for looking deeper, @cdaringe! We agree with your assessment that the incompatibility is with happy-dom using a non-prototypal instance field for isConnected where our polyfill guard is instead looking for a prototypal property.

We agree that the spec doesn't technically demand a prototypal property here; however, it does demand that the property be read-only, which in practice leads it to be present on Node.prototype in all of our supported environments documented in axe-core's browser support policy. I'll also note that the Node.prototype check is not something that axe-core has come up with uniquely; it is based on the MDN-recommended isConnected polyfill.

For fixing this specific incompatibility, we think it would be preferable to update happy-dom's implementation to be closer to how browsers and JSDom implement this property, for compatibility with the MDN-recommended polyfill strategy and with the DOM spec's requirement that the property be read-only. However, if the happy-dom folks push back on that, we'd be open to a contribution that updates our polyfill to be compatible, so long as it maintains compatibility with the existing cases it covers.

As far as an option to disable polyfills entirely: we're going to decline this feature suggestion (and so I'm going to close this issue). axe-core's polyfills are already surrounded with guards that no-op them in all supported environments that don't require them. A "no polyfills" option would only be applicable to environments that we explicitly do not support.

That said, we are interested in eventually updating axe-core to avoid having to use polyfills at all. Right now, most polyfills we use are there because we still support IE11; we'd like to evaluate whether we can avoid using polyfills at all in the future when we eventually drop IE11 support. However, we don't have a specific timeline to commit to for that right now.

@dbjorge dbjorge closed this as completed Jul 13, 2023
@cdaringe
Copy link
Author

No problem, thanks for writing back with a thorough response. I think it’s a fair take.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants