-
Notifications
You must be signed in to change notification settings - Fork 3
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!: fully native esm #2610
feat!: fully native esm #2610
Conversation
AviVahl
commented
Dec 8, 2024
- all packages are now "type": "module"
- replaced commonjs calls with their esm counterparts
- ensure all dynamic imports now use pathToFileURL to properly work on win32
- removed now-unneeded helper that bypassed typescript transpilation to get to the "real" import()
- changed default output format to esm
@@ -252,7 +252,7 @@ function connectWorkerToHost(envName: string, worker: ReturnType<typeof runWorke | |||
worker.postMessage(message.data); | |||
}; | |||
const handleInitializeError = (e: AnyMessage) => { | |||
rej(new Error(`failed initializing environment ${envName} with error message: ${JSON.stringify(e.data)}`)); | |||
rej(new Error(`failed initializing environment ${envName}`, { cause: e.data })); |
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 one actually prints the error now :)
ab8afb2
to
512e78b
Compare
- all packages are now "type": "module" - replaced commonjs calls with their esm counterparts - ensure all dynamic imports now use pathToFileURL to properly work on win32 - removed now-unneeded helper that bypassed typescript transpilation to get to the "real" import() - changed default output format to esm
512e78b
to
c3d4a4c
Compare
if one want to change this to esm, make sure that some bundle splitting is happening. | ||
*/ | ||
format: 'iife', | ||
format: 'esm', |
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.
@barak007 let's discuss this. I saw no other way to get native esm working downstream.
@@ -87,7 +87,7 @@ export async function runEngine({ | |||
outputPath = fs.resolve(rootDir, outputPath); | |||
|
|||
const jsOutExtension = '.js' as '.js' | '.mjs'; | |||
const nodeFormat = jsOutExtension === '.mjs' ? 'esm' : 'cjs'; | |||
const nodeFormat = 'esm' as 'esm' | 'cjs'; |
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 is interesting. basically forcing esm
@@ -130,6 +131,7 @@ export class NodeConfigManager { | |||
try { | |||
const text = outputFiles[0]!.text; | |||
const module = { exports: { default: undefined as any } }; | |||
const require = createRequire(import.meta.url); |
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.
not sure about this. we're in esm context now. manual evaluation with require?