Draft: Fix 12509: Match webpack/babel - evaluate dirname/filename at runtime when doing a build #14917
Conversation
|
updated the pr description to link to more issues it would close. see also #2865 |
| @@ -0,0 +1,53 @@ | |||
| import { describe, expect, it, beforeAll } from "bun:test"; | |||
There was a problem hiding this comment.
instead of manually testing, can you move these to using the itBundled function. you can use onAfterBundle to use expect on the output code.
| stdin: Bun.file("/dev/null"), | ||
| }); | ||
| const text = out.stdout.toString() | ||
| expect(text.includes("__dirname =")).toBe(true) |
There was a problem hiding this comment.
we should attempt to ensure that the value assigned to __dirname is the correct value.
| } else if (p.options.output_format == options.Format.esm) { | ||
| if (uses_dirname or uses_filename) { | ||
| const count = @as(usize, @intFromBool(uses_dirname)) + @as(usize, @intFromBool(uses_filename)); | ||
| var declared_symbols = DeclaredSymbol.List.initCapacity(p.allocator, count) catch unreachable; |
There was a problem hiding this comment.
tiny nit, can all newly added catch unreachable on memory allocation failures be rewritten to catch bun.outOfMemory()
| @@ -3507,7 +3508,7 @@ pub const Parser = struct { | |||
| // var __dirname = "foo/bar" | |||
| // var __filename = "foo/bar/baz.js" | |||
| // | |||
| if (p.options.bundle or !p.options.features.commonjs_at_runtime) { | |||
| if (p.options.target == Target.browser or !p.options.transform_only) { | |||
There was a problem hiding this comment.
why is target being passed for this? was the original version of this incorrect? i think the main issue is lowering __dirname to a string when bundling.
edit: oh, i didnt realize there was more string emitting code (i saw it deleted). i still find this code extremely odd because it emits absolute paths from the builder, which surely dont have any use in a browser.
There was a problem hiding this comment.
Maybe I should look more closely to when commonjs_at_runtime is true. To exactly match current tooling it seemed best to do this only when target was browser. Also I think in node later I use .dirname while in bun it was set to use "dir".
So I think the cases are
- Bun run ... - always inline
- Bun build --target="node" -> import.meta.dirname / path.basename(pathToFileURL(import.meta.url))
- Bun build --target="bun" -> import.meta.dir
- Bun build --target="browser" -> souce_path
- Bun build format=cjs -> do nothing
What does this PR do?
Fixes #12509
Fixes #13921
Fixes #4216
This PR tries to correctly evaluate
__dirnameand__filenamewhenever possible.Rather than almost always inlining them to the current source path. This PR changes the behavior to only do that on "run" not build since it doesn't match the behavior of webpack or babel and was bugged in the above issue.
Webpack:
target browser: inline __dirname
target esm: convert to esm compat
In webpack you can control the values or preserve them but this is the default.
Babel:
always preserve no matter the module target
How did you verify your code works?
I wrote test cases
bun-debug test test-file-name.test)