search

dequelabs / axe-core

Accessibility engine for automated Web UI testing
https://www.deque.com/axe/
Mozilla Public License 2.0
5.99k stars 779 forks source link

Consider not exporting global if AMD or CommonJS module definition succeeded. #2052

Open engelsdamien opened 4 years ago

engelsdamien commented 4 years ago

Expectation: When loading axe as an AMD module, I don't expect to also have a global variable named 'axe'

Actual: Axe always export unconditionally on window when in browser context.

Motivation: Exporting on window can cause axe to override an existing value assigned to this name ('axe' is quite short and could be realistically picked as a name by a bundler/minifier for large enough bundles)

Code is here: https://github.com/dequelabs/axe-core/blob/deed0f150680dccf20c3199410a6f00960a202dd/lib/core/index.js#L26

straker commented 4 years ago

Thanks for letting us know. We're actually in the works of converting the library to ES Modules and having something create that AMD / CommonJS code for us. Hopefully that will solve this problem.

engelsdamien commented 4 years ago

Sounds good, do you have a pointer to an issue I could follow ?

straker commented 4 years ago

We don't have an issue for it. Mostly has been tracked internally. But I can keep you posted i this issue on progress.

Andarist commented 4 years ago

@straker are there any particular challenges regarding that in the axe-core codebase? I potentially could help you migrate the codebase to ES modules - usually, most of the work can be automated, but maybe your codebase uses some unusual patterns?

straker commented 4 years ago

@Andarist The codebase is actually already converted to ES modules. Thanks for reminding me of this issue, it might be too late to get into the 4.0 release but I'll take a look.

Andarist commented 4 years ago

If I could do anything to help you ship this in 4.0 - let me know. I'm pretty proficient with modern bundling tools, build systems etc. I haven't worked with Grunt for a while though so I would propose replacing it with something else as it's not a popular tool nowadays.

straker commented 4 years ago

Alright, so since this was a breaking change it's too late to go into the 4.0 release. We'll have to revisit this issue when looking at 5.0.

Andarist commented 4 years ago

What's the estimate for v4? From what I see it has not been shipped yet, so maybe there still would be time for an external contributor to work on this one?

straker commented 4 years ago

4.0 is schedule to go out on Monday. Since this is a breaking change (global axe no longer being loaded if using AMD or CommonJS) we had our code freeze for breaking changes last week. So even with a pr, we wouldn't be able to merge it.

Andarist commented 4 years ago

With such a schedule I totally understand the code freeze. Thanks for letting me know!