-
Notifications
You must be signed in to change notification settings - Fork 43
chore!: use pure ES modules #590
Conversation
| "@babel/runtime": "^7.12.18", | ||
| "@netlify/eslint-config-node": "^3.3.9", | ||
| "ava": "^2.4.0", | ||
| "ava": "^3.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required for pure ES modules support.
|
|
||
| const client = getClient() | ||
| await t.throwsAsync(client.updateAccount(), "Missing required path variable 'account_id'") | ||
| await t.throwsAsync(client.updateAccount(), { message: "Missing required path variable 'account_id'" }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ava v3 changes the signature of t.throwsAsync().
| // any experimental flags | ||
| const require = createRequire(import.meta.url) | ||
|
|
||
| export const openApiSpec = require('@netlify/open-api') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@netlify/open-api exports a JSON file. However, importing JSON files with pure ES modules is not currently supported without experimental flags.
We could make @netlify/open-api export a JavaScript file instead.
Instead, this PR opts for a faster approach: using module.createRequire() until JSON imports become possible.
Please also note that import.meta.resolve() is not possible since openApiSpec must be available synchronously.
0fa647b to
8efdbc2
Compare
8efdbc2 to
bda518d
Compare
| return sites | ||
| } | ||
| const client = new NetlifyAPI('1234myAccessToken') | ||
| const sites = await client.listSites() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Top-level await is possible with pure ES modules.
bda518d to
e2feec7
Compare
| flags: ${{ steps.test-coverage-flags.outputs.os }},${{ steps.test-coverage-flags.outputs.node }} | ||
| - name: Build | ||
| run: npm run build | ||
| if: "${{ matrix.node-version == '*' }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #589
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧑🚀 next step to the future!
|
This is a terrible idea. This is node breaking if our ENTIRE app is cjs. |
|
Hi @AlanGreyjoy, |
|
@ehmicky , |
Fixes #495
This switches from CommonJS to pure ES modules.
This is a breaking change, since consumers that still use CommonJS will need to use a dynamic
import()to load this module instead ofrequire(). Consumers that use pure ES modules will not need to change anything.