Skip to content
This repository was archived by the owner on Oct 10, 2022. It is now read-only.

Conversation

@ehmicky
Copy link
Contributor

@ehmicky ehmicky commented Nov 29, 2021

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 of require(). Consumers that use pure ES modules will not need to change anything.

@ehmicky ehmicky added the type: chore work needed to keep the product and development running smoothly label Nov 29, 2021
@ehmicky ehmicky self-assigned this Nov 29, 2021
"@babel/runtime": "^7.12.18",
"@netlify/eslint-config-node": "^3.3.9",
"ava": "^2.4.0",
"ava": "^3.0.0",
Copy link
Contributor Author

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'" })
Copy link
Contributor Author

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')
Copy link
Contributor Author

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.

return sites
}
const client = new NetlifyAPI('1234myAccessToken')
const sites = await client.listSites()
Copy link
Contributor Author

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.

flags: ${{ steps.test-coverage-flags.outputs.os }},${{ steps.test-coverage-flags.outputs.node }}
- name: Build
run: npm run build
if: "${{ matrix.node-version == '*' }}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #589

Copy link
Contributor

@lukasholzer lukasholzer left a 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!

@ehmicky ehmicky merged commit ba87fb6 into main Nov 30, 2021
@ehmicky ehmicky deleted the chore/pure-esm branch November 30, 2021 13:48
@AlanGreyjoy
Copy link

This is a terrible idea. This is node breaking if our ENTIRE app is cjs.

@ehmicky
Copy link
Contributor Author

ehmicky commented Dec 18, 2021

Hi @AlanGreyjoy,
This was made with a major release, so this should not be breaking your application.
You can still upgrade to that major release by using a dynamic import() to import netlify, which works even if your project is using CommonJS.

@AlanGreyjoy
Copy link

@ehmicky ,
What in tarnation.... 5 years as a full stack engineer and I had no idea you could use dynamic import() on es modules.... I need to put myself in the corner.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

type: chore work needed to keep the product and development running smoothly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use pure ES modules

4 participants