Fix default export returning undefined in ESM #440
Conversation
b26f6ac to
e4f1cc0
Compare
fcaa531 to
01d6175
Compare
01d6175 to
111274d
Compare
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
smoke-tests/smoke-test-cjs.cjs
Outdated
| // 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
smoke-tests/smoke-test-cjs.cjs
Outdated
| 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?
smoke-tests/smoke-test-cjs.cjs
Outdated
| ); | ||
|
|
||
| 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)
0de1d19 to
ee85301
Compare
|
Not sure if this is the preferred solution. I think this could be properly tackled by #399 instead. |
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?