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
feat!: convert to ESM and vitest #1230
Conversation
β± Benchmark resultsComparing with dec4d3f largeDepsEsbuild: 5.5sβ¬οΈ 35.31% decrease vs. dec4d3f
LegendlargeDepsNft: 29.3sβ¬οΈ 24.58% decrease vs. dec4d3f
LegendlargeDepsZisi: 41.2sβ¬οΈ 35.26% decrease vs. dec4d3f
|
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.
β€οΈ vitest π―
@@ -17,7 +17,7 @@ jobs: | |||
with: | |||
token: ${{ steps.get-token.outputs.token }} | |||
release-type: node | |||
package-name: '@netlify/@netlify/zip-it-and-ship-it' | |||
package-name: '@netlify/zip-it-and-ship-it' |
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.
how did this worked π
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.
No Idea
@@ -19,4 +20,9 @@ const runBenchmarks = async function () { | |||
console.log(`${largeDepsEsbuild}ms`) | |||
} | |||
|
|||
runBenchmarks() | |||
try { | |||
await runBenchmarks() |
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.
Just as information you could now use the vitest bench
benchmarking functionality. Nothing to change here but just as a FYI
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.
Yeah, I was wondering, but didn't want to add even more changes.
Not super happy with vitest yet, had to do some workarounds, so do not want to buy in yet fully :)
@@ -8,6 +8,7 @@ import endOfStream from 'end-of-stream' | |||
|
|||
export { Archiver as ZipArchive } from 'archiver' | |||
|
|||
// TODO make enum |
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.
Still todo?
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.
Yes, afterwards.
// @ts-expect-error vitest with esbuild already resolves the default export, whereas Node.js does not | ||
const resolveDependency: typeof nftResolveDependency.default = nftResolveDependency.default | ||
? nftResolveDependency.default | ||
: nftResolveDependency |
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.
Reported here: vitest-dev/vitest#2219
export const importFunctionFile = async function <T = any>(functionPath: string): Promise<T> { | ||
// We use relative paths here, because vitest cannot handle absolute paths or urls :( | ||
// eslint-disable-next-line import/no-dynamic-require | ||
const result = await import(`/@fs/${functionPath}`) |
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.
Reported here vitest-dev/vitest#2218
π Thanks for submitting a pull request! π
Summary
Fixes #749
This converts zisi to pure ESM. It also migrates from ava to vitest.
For us to review and ship your PR efficiently, please perform the following steps:
This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing
a typo or something that`s on fire π₯ (e.g. incident related), you can skip this step.
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)