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

fix(packages/cli): revert ESM #730

Merged
merged 10 commits into from
May 30, 2023
Merged

fix(packages/cli): revert ESM #730

merged 10 commits into from
May 30, 2023

Conversation

Zidious
Copy link
Contributor

@Zidious Zidious commented May 17, 2023

This PR reverts the recent conversion to ESM for CLI as it's not required which should fix the pathing for the CLI binary when looking for the axe-core source.

Closes: #729
Closes: #726

Copy link
Member

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be more complicated than:

export const getAxeSource = (axePath = require.resolve('axe-core')): string => {
  return fs.readFileSync(axePath, 'utf-8')
}

...and if not, do we really need this function?

packages/cli/package.json Outdated Show resolved Hide resolved
@Zidious Zidious marked this pull request as ready for review May 18, 2023 14:05
@Zidious Zidious requested a review from a team as a code owner May 18, 2023 14:05
Copy link
Member

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but as said before, there was probably a reason we did funky path lookups. Please make sure that there isn't some obscure requirement we're breaking by using require.resolve().

@stephenmathieson
Copy link
Member

Should we remove the ESM stuff in the CLI package (since it's not used/needed)?

@michael-siek
Copy link
Member

I think we should not support ESM in CLI

@Zidious
Copy link
Contributor Author

Zidious commented May 23, 2023

I think we should not support ESM in CLI

I am in favour of that - but IMO it should happen outside of this PR

@Zidious Zidious changed the title fix(packages/cli): ensure we find axe.js within CLI fix(packages/cli): revert ESM May 26, 2023
@Zidious Zidious requested review from stephenmathieson and a team May 26, 2023 02:30
@Zidious Zidious enabled auto-merge (squash) May 30, 2023 16:06
@Zidious Zidious merged commit 4e1fb95 into develop May 30, 2023
@Zidious Zidious deleted the fix/axe-core-cli-sorce-path branch May 30, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants