Inject __dirname for commonjs compatibility instead of banner#94
Inject __dirname for commonjs compatibility instead of banner#94adcreare wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the esbuild bundling prelude logic to provide CommonJS-compatibility __dirname via esbuild’s inject mechanism (rather than defining __dirname directly in a banner), avoiding collisions when downstream dependencies also introduce top-level __dirname (per issue #93).
Changes:
- Removes
__dirnamecomputation from the esbuild banner and instead injects an__dirnameprovider module when bundling (outFileset). - Refactors the TypeScript config root path calculation into a
packageRootconstant. - Updates compile bundling tests to expect the injected
__dirnameprelude and adds coverage for a project-level__dirnamedeclaration coexisting with injected usage.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/compile.ts | Switches __dirname from a banner-defined constant to an esbuild injected symbol to avoid naming collisions in bundled output. |
| src/compile.spec.ts | Updates expected bundle prelude output and adds a test ensuring local __dirname declarations don’t break injected __dirname usage. |
| package.json | Bumps package version to 10.2.0. |
| package-lock.json | Updates lockfile version fields to match 10.2.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const dirNameJsCompatabilityInjection = `import path from 'node:path'; | ||
| export const __dirname = path.dirname(__filename);`; // relies on __filename from commonJsCompatabilityBanner | ||
| const dirNameJsCompatabilityInjectionURL = `data:text/javascript,${encodeURIComponent(dirNameJsCompatabilityInjection)}`; |
|
❌ PR review status - has 1 reviewer outstanding |
|
Beta Published - Install Command: |
carlansley
left a comment
carlansley
left a comment
There was a problem hiding this comment.
behaviorally lgtm, just a couple of minor things
| ); | ||
|
|
||
| const output = await import(path.join(outDir, 'index.mjs')); | ||
| assert.deepEqual( |
There was a problem hiding this comment.
this would be simpler:
assert.equal(output.injectedDirname, await fs.realpath(outDir));
assert.equal(output.localDirname, 'declared in project');
| typescript.sys, | ||
| // @checkdigit/typescript-config package root: | ||
| path.dirname(path.dirname(fileURLToPath(import.meta.url))), | ||
| packageRoot, |
There was a problem hiding this comment.
do we need to have this run at the module level? not sure I understand this change
Closes #93