Skip to content

feat: fork cross-spawn to fix esbuild chaos#25

Closed
43081j wants to merge 1 commit intomainfrom
cs-gone
Closed

feat: fork cross-spawn to fix esbuild chaos#25
43081j wants to merge 1 commit intomainfrom
cs-gone

Conversation

@43081j
Copy link
Copy Markdown
Member

@43081j 43081j commented Aug 8, 2024

esbuild isn't great at bundling for node. it currently produces invalid code by leaving require calls in the esm output, which is far from great.

To resolve this, this moves cross-spawn into the repo and converts it to ESM while dropping any logic we weren't making use of.

DRAFT until i finish adding the tests for the cross-spawn module.

This may also be discarded if we find a simpler way to get esbuild to output valid code.

cc @bluwy

esbuild isn't great at bundling for node. it currently produces invalid
code by leaving require calls in the esm output, which is far from
great.

To resolve this, this moves cross-spawn into the repo and converts it to
ESM while dropping any logic we weren't making use of.
MIT License

Copyright (c) 2024 Tinylibs
Copyright (c) 2018 Made With MOXY Lda <hello@moxy.studio>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Whats that?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

the license of cross-spawn. any usage of the original code needs to maintain the copyright

@benmccann
Copy link
Copy Markdown
Contributor

I feel like the better solution to this would be to switch from tsup to rollup

@43081j
Copy link
Copy Markdown
Member Author

43081j commented Aug 26, 2024

are we sure rollup doesn't have the same problem? under the hood, esbuild is at fault here. it just doesn't transform require() calls of built in modules. so the output has CJS in it still and definitely shouldn't

would be curious if rollup just does the same, i feel like someone pointed me at an issue in their github that suggested it does 👀

@benmccann
Copy link
Copy Markdown
Contributor

I know that rollup doesn't have this issue because we just tried to migrate a repo from rollup to rolldown and ran into an issue where the require was no longer being transformed (rolldown/rolldown#2041)

@bluwy
Copy link
Copy Markdown
Contributor

bluwy commented Aug 27, 2024

I tried rollup locally when we discussed this issue and it didn't have the same problem IIRC, which i thought it would have, so using rollup would definitely be an option, but it requires more setup.

@benmccann
Copy link
Copy Markdown
Contributor

I sent a PR for rollup here: #35

@gameroman
Copy link
Copy Markdown

Since it is using tsdown now and there is also #100 open this can be closed now probably

@43081j
Copy link
Copy Markdown
Member Author

43081j commented Mar 30, 2026

we still need to get rid of cross spawn eventually but this PR is unlikely to be the one, so will close.

@43081j 43081j closed this Mar 30, 2026
@43081j 43081j deleted the cs-gone branch March 30, 2026 23:07
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.

5 participants