-
Notifications
You must be signed in to change notification settings - Fork 340
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
chore: migrate more codebase to ESM #5267
Conversation
📊 Benchmark resultsComparing with 2b5d96c Package size: 247 MB⬇️ 0.00% decrease vs. 2b5d96c
Legend
|
|
||
// 12 hours | ||
const UPDATE_CHECK_INTERVAL = 432e5 | ||
const pkg = JSON.parse(readFileSync(fileURLToPath(new URL('../package.json', import.meta.url))), 'utf-8') | ||
const pkg = await getPackageJson() |
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.
This was extracted into a separate file, so we do not have to do this all over again multiple times.
import getGlobalConfig from '../utils/get-global-config.cjs' | ||
import { openBrowser } from '../utils/open-browser.cjs' | ||
import StateConfig from '../utils/state-config.mjs' | ||
import { identify, track } from '../utils/telemetry/index.mjs' |
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.
I started unusing /utils/index.cjs
, and instead import utils from individual files :
- It is easier to migrate
- It makes node not require everything at once, which might come in handy once the commands are not all loaded always, and only the command in use.
So the goal would be to remove utils/index.cjs
at some point, but if anyone feels opposed to that it can also be reinstated at the end of the migration.
]) | ||
t.deepEqual(watchDirs, [path.join(builder.directory, 'functions'), path.join(builder.directory, 'utils')]) | ||
}) | ||
}) |
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.
getFunctionsAndWatchDirs
was unused, so removed.
const functionsWithProps = functions.filter(({ runtime }) => runtime === JS).map((func) => addFunctionProps(func)) | ||
|
||
return { functions: functionsWithProps, watchDirs } | ||
} |
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.
getFunctionsAndWatchDirs
was unused so removed
packageJson = JSON.parse(await readFile(packageJsonPath)) | ||
} | ||
|
||
return packageJson |
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.
This now also caches the root package.json
of the cli as I do not think it will change during runtime.
Summary
Migrates some parts of the codebase to ESM.
To move further I have to first introduce vitest for the unit tests to be able to properly mock modules, which rewiremock fails with esm now.
For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)