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

Update examples in docs to use ES Modules #2954

Merged
merged 4 commits into from
Feb 13, 2022
Merged

Update examples in docs to use ES Modules #2954

merged 4 commits into from
Feb 13, 2022

Conversation

scottdotweb
Copy link
Contributor

I'm trying AVA for the first time and this appears to be the case as of 4.0.

I'm trying AVA for the first time and this appears to be the case as of 4.0.
@scottdotweb scottdotweb changed the title Update first example to import test, not require Update first example in 01-writing-tests.md to import test, not require Jan 24, 2022
@novemberborn
Copy link
Member

Either work, it depends on whether you're using .mjs files or have set "type": "module" in your package.json. There's a lot of other require statements in the documentation, we should make them consistent.

I do lean towards the ESM syntax, but then I wonder if that'll trip people up who have a default setup of CommonJS. @sindresorhus what do you think?

@sindresorhus
Copy link
Member

I would use ESM syntax.

@novemberborn
Copy link
Member

@scottdotjs would you be up for converting all the docs? I'd understand if not — but it'd be good to keep the syntax consistent so I'd rather do it all in one go.

@scottdotweb
Copy link
Contributor Author

Sure! This PR only covered the first example as that was as far as I'd for at the time. 🙂

@scottdotweb scottdotweb changed the title Update first example in 01-writing-tests.md to import test, not require Update examples in docs to use ES Modules Feb 8, 2022
@scottdotweb
Copy link
Contributor Author

OK, I've changed the examples to use ES Modules as far as was obvious to me - in the case of other packages, I checked that it worked for them by consulting their issues if it wasn't shown in their documentation, and/or tried it myself. There were several instances I didn't change, however:

06-configuration.md

You can require() dependencies.
[...]
const baseConfig = require('./ava.config.cjs');

JSON imports aren't quite there yet (as I'm sure you know) so it's your call on how to replace this if you want to. There's another JSON file being required in recipes\when-to-use-plan.md.

testing-with-selenium-webdriverjs.md

require('chromedriver');

I've never used require() on its own like this so not sure what the import equivalent would be.

vue.md

const hooks = require('require-extension-hooks');
const Vue = require('vue');
const Component = require('component.vue');

I have no Vue experience either so couldn't update this section; also the Vue basic examples don't show Vue being used this way but rather all as import { createApp } from 'vue', so I don't know if your example is out of date or doing something different.

@novemberborn
Copy link
Member

06-configuration.md

You can require() dependencies.
[...]
const baseConfig = require('./ava.config.cjs');

This one's fine, it's an example with a CJS file after all.

JSON imports aren't quite there yet (as I'm sure you know) so it's your call on how to replace this if you want to. There's another JSON file being required in recipes\when-to-use-plan.md.

I'll push a commit to replace that with a read-file-sync.

require('chromedriver');

I've never used require() on its own like this so not sure what the import equivalent would be.

Just import 'chromedriver'; 😄

vue.md

const hooks = require('require-extension-hooks');
const Vue = require('vue');
const Component = require('component.vue');

I have no Vue experience either so couldn't update this section; also the Vue basic examples don't show Vue being used this way but rather all as import { createApp } from 'vue', so I don't know if your example is out of date or doing something different.

Yea I don't know — but require-extension-hooks implies it needs to be CJS.

@novemberborn novemberborn merged commit 576f534 into avajs:main Feb 13, 2022
@novemberborn
Copy link
Member

Thanks @scottdotjs!

@scottdotweb
Copy link
Contributor Author

Just import 'chromedriver'; 😄

Ohhh 😂 Well when you put it like that...

Thanks @scottdotjs!

You're welcome!

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

Successfully merging this pull request may close these issues.

3 participants