Skip to content
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

Fix default export returning undefined in ESM #440

Closed
wants to merge 3 commits into from

Conversation

Codex-
Copy link

@Codex- Codex- commented Sep 10, 2023

I noticed this previously but it's consistently reproducible with Bun.

The esm based index.mjs export for concurrently, export default concurrently.default, in some cases depending on the way you're executing concurrently can have the default function on concurrently itself which makes this export not work as expect, as default is undefined.

This PR adds a simple fallback export default concurrently.default || concurrently that allows some defense against this behaviour.

I have added tests for both Node and Bun that test the commonjs and esm import behaviour, as well as a direct ts import 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?

@coveralls
Copy link

coveralls commented Sep 10, 2023

Coverage Status

coverage: 99.31%. remained the same when pulling ee85301 on Codex-:fix_bun_compatibility into 1618208 on open-cli-tools:main.

@Codex- Codex- force-pushed the fix_bun_compatibility branch 2 times, most recently from fcaa531 to 01d6175 Compare September 10, 2023 23:17
Copy link
Member

@gustavohenke gustavohenke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this! I have mostly nitpicks, no questions on the intended change

Comment on lines 21 to 22
// Prevent an unhandled rejection, exit gracefully.
process.exit(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a difference?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is catching the error necessary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@paescuj
Copy link
Collaborator

paescuj commented Sep 19, 2023

Not sure if this is the preferred solution. I think this could be properly tackled by #399 instead.
Also I think it's about time to go full ESM, things are getting messy.
I'll look into this more detailed later on this day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants