Skip to content
This repository has been archived by the owner on Mar 19, 2021. It is now read-only.

docs: Fix example in README #18

Merged
merged 2 commits into from Jan 18, 2019
Merged

docs: Fix example in README #18

merged 2 commits into from Jan 18, 2019

Conversation

AdnoC
Copy link
Contributor

@AdnoC AdnoC commented Jan 17, 2019

Example didn't account for axe-puppeteer exporting multiple things. Also added comments about the context of the example code. Seems like export default stopped working at some point, since I know the code used to work.

Closes issue:

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: << Name here >>

README.md Outdated
const puppeteer = require('puppeteer')

// ... In async function
Copy link
Member

Choose a reason for hiding this comment

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

It would probably be best to just put this code in an async fn so that people can copy/paste and run without modifying anything.

What do you think?

@@ -22,9 +22,10 @@ This module uses a chainable API to assist in injecting, configuring and analyzi
Here is an example of a script that will drive Puppeteer to this repository, perform analysis and then log results to the console.

```js
const AxePuppeteer = require('axe-puppeteer')
const { AxePuppeteer } = require('axe-puppeteer')
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice if this wasn't required. Can we just do module.exports = AxePuppeteer instead? I know TS will complain, but we can just disable the linter for the added line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that then we can't export anything else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which means no access to the loadPage function.

Copy link
Member

Choose a reason for hiding this comment

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

Can we do something like:

// The below is to ensure CommonJS `require()` works as expected.
// tslint:disable-next-line
exports = module.exports = AxePuppeteer

// ESM exports.
export { loadPage }
export default AxePuppeteer

Note that I haven't tested this.

Copy link
Member

Choose a reason for hiding this comment

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

I did a bit of testing and this will work fine. See this TypeScript playground example for proof.

Note that module.exports and exports.default are set there, as well as exports.loadPage.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, I see, exports is overwritten after we set exports.loadPage. Disregard :)

@AdnoC AdnoC merged commit 5bd5b64 into develop Jan 18, 2019
@AdnoC AdnoC deleted the docs-readme-example branch January 18, 2019 17:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants