dequelabs / axe-webdriverjs

Provides a chainable axe API for Selenium's WebDriverJS and automatically injects into all frames.
Mozilla Public License 2.0
130 stars 46 forks source link

Move axe-core back into dependencies #17

Closed marcysutton closed 7 years ago

marcysutton commented 7 years ago

Right now it requires you to install it separately but we never updated the docs to reflect that. I think we should actually move it back in as a dependency to make axe-webdriverjs a quicker install.

dylanb commented 7 years ago

Why not update the documentation?

marcysutton commented 7 years ago

It's a lot more convenient to have axe-core installed automatically with this library. With the change @WilcoFiers just made to pass a different source as an option, we can support custom builds without requiring every dev consumer to install two packages.

WilcoFiers commented 7 years ago

Agreed. Now that we can pass the axe source in as a param, users can still manage which version of axe they are using if they want to. But for users who don't care much and just want the latests it's easier if axe-core is a regular dependency instead of a peer dependency. But yes, documentation update is needed. Let me go and sort that out :)

dylanb commented 7 years ago

how do we make axe-core a dependency and not break attest-webdriverjs?

marcysutton commented 7 years ago

Can we port the change Wilco made in axe-webdriverjs to attest-webdriverjs?

dylanb commented 7 years ago

See discussion in that PR

marcysutton commented 7 years ago

Before we release an update for this module I want to have one last conversation about axe-core as a dependency.

A little background axe-core is included as a devDependency in axe-webdriverjs, I assume for easy testing. But it's also included as a peerDependency to allow attest-webdriverjs users to install an independent, potentially newer axe-core version (as part of Attest). After doing some investigating, I found that attest-webdriverjs includes axe-core as an explicit dependency–so it would kick off a download of axe-core independent of the axe version in this module.

The important part The version of axe-core available to attest-webdriverjs users would typically have a higher number, so it would be downloaded by that module via Artifactory. If the open source version number happens to be higher for a short period of time, Attest users would get the newer open source version. Which seems okay to me–wouldn't paying customers want the latest code available at all times? Open source users would continue to get the latest public version without the extra install step. Seems like a win-win for everyone.

kaelig commented 7 years ago

We're having issues on a project because of this peerDependency situation. I'd really like it to be included as a dependency.

The issue is that if you try to npm shrinkwrap in a project where axe-webdriverjs is a devDependency, you get this error message:

peer invalid: axe-core@~2.0.5, required by axe-webdriverjs@0.3.0

cc @sitaggart

marcysutton commented 7 years ago

@kaelig I think that issue will be resolved with the fix that went in with #20.

kaelig commented 7 years ago

Thanks @marcysutton. I'm not sure as I've tried many ways of installing axe-core as a devDependency by locking the version number, which didn't solve the issue. I'll keep you posted.

marcysutton commented 7 years ago

I think we should revisit this now that we have one active axe-core repository, especially since axe-webdriverjs takes a source option. Here's why:

axe-cli triggers warn.js for a "missing" axe-core in axe-webdriverjs even though it includes aXe as a dependency. I tried addressing that by looking up the tree for axe-core with an npm module called parent-package-json, but the calling project (axe-cli, in this case) then would need parent-package-json to be installed to run warn.js.

IMO it makes a lot more sense to remove lib/warn.js, make axe-core a dependency of this project, and let attest-webdriverjs override it with the custom source option. I did a bit of testing on it by updating the dependency and the tests still pass in attest-webdriverjs. @dylanb what do you think?

dylanb commented 7 years ago

@marcysutton yep - go for it!

marcysutton commented 7 years ago

Fixed with 1aebfe7.