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: remove package.json production exports #137

Merged
merged 2 commits into from
Dec 11, 2024

Conversation

marbemac
Copy link
Contributor

@marbemac marbemac commented Dec 5, 2024

@affects atoms, core, immer, machines, react

Description

The node resolver chooses the first package.json export that matches. Since the "production" export was not listed first, it has effectively not been in use (at least when using the node resolver and any tooling that adheres to that resolving strategy). Much more of a write-up and how I ran into this in nitrojs/nitro#2923

I've adjusted the exports to move "production" to the first position.

That said - is there a reason to ship the minified files at all? Why not remove the complexity here and allow downstream consumers to consume as is (node) or minify w their existing build pipeline (vite, swc, etc)? Better stack traces / debugging is a major benefit to downstream consumers importing a non-minified version of zedux.

@bowheart
Copy link
Collaborator

bowheart commented Dec 6, 2024

@marbemac nice! Thanks for this.

I'm curious about this part, from your nitro comment:

This results in a mismatch, and runtime error when importing the affected package(s).

So something was managing to import the production build while nitro (properly) wasn't? Any details you can give about what caused a mismatch or what the runtime error was would be helpful.

I must confess, I'm not sure what production exports are usually used for. I added those mostly for reference once so people could find the minified builds. Those minified builds, btw, are usually used with a cdn like unpkg for optimal content delivery.

We import Zedux exactly as you said,

consume as is (node) or minify w their existing build pipeline

which is Vite for us. And I'm not sure why anyone would want to do it differently. At least all modern build tools I know minify it for you and, as you said, therefore give you better, consistent source maps for a much better debugging experience even in prod than you could possibly get from importing pre-minified code for your prod build.

If something was able to pull those in, then I think I'm in favor of removing those production exports altogether rather than ensuring all build tools use them

@marbemac
Copy link
Contributor Author

marbemac commented Dec 6, 2024

So something was managing to import the production build while nitro (properly) wasn't? Any details you can give about what caused a mismatch or what the runtime error was would be helpful.

So admittedly I'm not familiar with Nitro internals beyond what I looked at yesterday, but here's what I think is going on.

Nitro supports building for many different target runtimes. As part of that build process, they use https://github.com/vercel/nft to identify the specific files in a target dependency that matter for the target runtime it's building for, and only move those files over to the build output's node_modules folder. https://github.com/vercel/nft is correctly adhering to node's resolver behavior (so it did not leverage zedux production export, and as a result all the files relevant to the regular "import" condition were identified and copied to build output).

Additionally, Nitro re-writes the exports in the dependency's package.json, and prioritizes the production export specifically (https://github.com/nitrojs/nitro/blob/v2/src/rollup/plugins/externals.ts#L485), NOT adhering to the node resolvers behavior. So in the zedux case, it re-wrote all the exports to point to the prod path, but the prod file was not copied over by nft - resulting in a mismatch at runtime. If that makes any sense.

If something was able to pull those in, then I think I'm in favor of removing those production exports altogether rather than ensuring all build tools use them

This makes the most sense to me - should I update the PR to just remove the prod export?

@bowheart
Copy link
Collaborator

bowheart commented Dec 6, 2024

should I update the PR to just remove the prod export?

@marbemac Yes. I've thought about it and I'm sure that's the safest thing to do right now. Can always revisit later if we find a better use for the production export

Nitro re-writes the exports in the dependency's package.json, and prioritizes the production export specifically

That sounds incorrect to me. I'd be surprised if Zedux was the first package that ran into problems from that. But since we can update easily on our end, let's just do that.

@marbemac
Copy link
Contributor Author

Alrighty, I've removed the production exports.

That sounds incorrect to me. I'd be surprised if Zedux was the first package that ran into problems from that.

Agree - and sounds like they do as well, they re-opened the issue.

@marbemac marbemac changed the title fix: hoist package.json conditional exports fix: remove package.json production exports Dec 10, 2024
@bowheart
Copy link
Collaborator

@marbemac these CI failures were not related to your changes. Looks like the v1 branch is missing some dep updates for its tests. I added those in #138, which just merged into the v1 branch. Can you rebase your branch off v1.x and force push? I'll merge this and get a new v1 version published as soon as you can get to that.

@marbemac marbemac force-pushed the mbm/fix-package-exports branch from abacd12 to 5d2f23b Compare December 11, 2024 20:49
@marbemac
Copy link
Contributor Author

done and done

@bowheart bowheart merged commit 09cdac5 into Omnistac:v1.x Dec 11, 2024
2 checks passed
@bowheart
Copy link
Collaborator

bowheart commented Dec 11, 2024

@marbemac Awesome! I've published v1.3.0. It's a minor version instead of a patch since #138 also updated v1 to support React 19 officially. That change is backwards compatible from Zedux's perspective (I believe I was actually mistaken in my changelog notes for the v1.3.0 release candidates) so it is not a major version, but it's at least a minor.

Since it was published from the v1.x support branch, it has the npm tag lts instead of latest. Install via npm i @zedux/react@lts or similar. I'll get relevant changes cherry picked to master.

bowheart pushed a commit that referenced this pull request Dec 11, 2024
bowheart added a commit that referenced this pull request Dec 11, 2024
@affects atoms, core, immer, machines, react

Co-authored-by: Marc MacLeod <[email protected]>
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.

2 participants