Fix default export returning undefined in ESM #440
Fix default export returning undefined in ESM #440Codex- wants to merge 3 commits intoopen-cli-tools:mainfrom
Conversation
Codex-
force-pushed
the
fix_bun_compatibility
branch
from
b26f6ac to
e4f1cc0
Compare
Codex-
force-pushed
the
fix_bun_compatibility
branch
2 times, most recently
from
fcaa531 to
01d6175
Compare
Codex-
force-pushed
the
fix_bun_compatibility
branch
from
01d6175 to
111274d
Compare
gustavohenke
left a comment
gustavohenke
left a comment
There was a problem hiding this comment.
Thanks for fixing this! I have mostly nitpicks, no questions on the intended change
| // Prevent an unhandled rejection, exit gracefully. | ||
| process.exit(1); |
There was a problem hiding this comment.
non-zero exit codes will fail the CICD, there were cases in my own testing where simply using process.exit() would cause a false success on the workflow run
| console.info('Imported cjs successfully'); | ||
| } catch (error) { | ||
| console.error(error); | ||
| console.debug(error.stack); |
There was a problem hiding this comment.
Node logs the stacktrace by default, doesn't bun do the same? E.g. if you make the assertion fail, won't it print the stack trace twice?
| ); | ||
|
|
||
| console.info('Imported cjs successfully'); | ||
| } catch (error) { |
There was a problem hiding this comment.
Why is catching the error necessary?
There was a problem hiding this comment.
unhandled rejections can fail silently, but these don't actually need to be async so I'll update them (ported from a larger suite I use for one of my own packages)
Codex-
force-pushed
the
fix_bun_compatibility
branch
from
0de1d19 to
ee85301
Compare
|
Not sure if this is the preferred solution. I think this could be properly tackled by #399 instead. |
4 participants
I noticed this previously but it's consistently reproducible with Bun.
The esm based
index.mjsexport forconcurrently,export default concurrently.default, in some cases depending on the way you're executingconcurrentlycan have the default function onconcurrentlyitself which makes this export not work as expect, asdefaultis undefined.This PR adds a simple fallback
export default concurrently.default || concurrentlythat allows some defense against this behaviour.I have added tests for both Node and Bun that test the
commonjsandesmimport behaviour, as well as a directtsimport case testing both for Bun's case.Also, I've noticed Node 19 is targetted here but Node 20 enters LTS next month, should this target 20 instead of 19?