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

Upgrade npm-run-path #1156

Merged
merged 1 commit into from
Sep 16, 2024
Merged

Upgrade npm-run-path #1156

merged 1 commit into from
Sep 16, 2024

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented Sep 16, 2024

This upgrades npm-run-path.

https://github.com/sindresorhus/npm-run-path/releases/tag/v6.0.0

For users, this means when Execa manipulates the PATH environment variable (due to execaNode() or to preferLocal: true), it might now do it slightly better in some edge cases.

@sindresorhus sindresorhus merged commit ba483e7 into main Sep 16, 2024
14 checks passed
@sindresorhus sindresorhus deleted the upgrade-npm-run-path branch September 16, 2024 19:19
@ehmicky
Copy link
Collaborator Author

ehmicky commented Sep 16, 2024

I'm thinking of doing a minor release for this PR, and also to give a small shout-out to nano-spawn in the release notes. What do you think?

@sindresorhus
Copy link
Owner

👍

@ehmicky
Copy link
Collaborator Author

ehmicky commented Sep 16, 2024

@rafael-lua
Copy link

rafael-lua commented Oct 6, 2024

@ehmicky Sorry to ping you here, not sure if it is worth its own issue, but this bump may trigger: sindresorhus/globby#260

I was trying to do a bundle including the dependencies with unbuild/rollup, and execa was a blocker in the release 9.4.0 because the new npm-run-path version.

@ehmicky
Copy link
Collaborator Author

ehmicky commented Oct 6, 2024

@rafael-lua Thanks for reporting this.

@sindresorhus This is a problem with https://github.com/sindresorhus/unicorn-magic. It is being discussed with sindresorhus/globby#260 too, but basically would impact any package using unicorn-magic directly or indirectly. With this PR, Execa started depending on unicorn-magic.

Basically, the issue is that unicorn-magic is using conditional exports, i.e. exports.node and exports.default in the package.json: https://github.com/sindresorhus/unicorn-magic/blob/6614e1e82a19f41d7cc8f04df7c90a4dfe781741/package.json#L14

To make this work with some bundlers requires consumers to specify node as the target. This seems to include both Webpack and Vite.

So, the problem is actually that consumers are misconfiguring their bundler. If they're using Node.js-specific code, they should specify that Node is the target. Which means that, one could argue, there's nothing wrong we're doing here, in principle.

That being said, the above issue has quite many upvotes and it does seem to be causing some headaches to some users. It's fair to say that configuring bundlers is not always easy (especially Webpack) and it creates some confusion. So maybe there's some small fix we could do to help users out here?

To me, it seems like package.json conditional exports are more helpful when some export is available in two versions: one for Node, one for browsers. However, in unicorn-magic's case, the default and node entrypoints export different methods. Which makes me wonder: shouldn't this be two separate packages instead? An alternative (mentioned in the bottom of that comment) would be to use two entrypoints instead in the package.json. Doing so would solve this problem.

What are your thoughts on this @sindresorhus?

@sindresorhus
Copy link
Owner

That being said, the above issue has quite many upvotes and it does seem to be causing some headaches to some users. It's fair to say that configuring bundlers is not always easy (especially Webpack) and it creates some confusion. So maybe there's some small fix we could do to help users out here?

It's tempting to do that, but if packages always work around bad bundler DX, the bundlers will never have an incentive to improve.

To me, it seems like package.json conditional exports are more helpful when some export is available in two versions: one for Node, one for browsers.

I don't see why it would have to be limited to that.

Which makes me wonder: shouldn't this be two separate packages instead?

Why should it? Isomorphic packages is intended use-case for conditional exports.

An alternative (mentioned in the bottom of that comment) would be to use two entrypoints instead in the package.json.

If anyone feels strongly enough to do the work, I would merge it, but it's also not something I plan to spend time on.

@JonParton
Copy link

Just to throw in - as a user of execa (Thank you so much for creating / maintaining! It's awesome! 🙏) I just struggled for several hours due to this issue too 😫

Managed to eventually track down this was due to the unicorn-magic imports, played around patching the package.json but ultimately had to downgrade my execa version to one i knew worked...

For context on Bundler configurations etc ... In this case I am using exaca within a next.js project, however it is not being used as part of the application code, but being used as part of some dev scripts that get run through tsx so it is not being bundled at all yet still encounters the dreaded error 😔.

Couldn't figure out in what way I could effect its imports in this setup, so had to give up and downgrade.

Just wanted to highlight - Still causing pain!

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