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

Parcel production builds can't bundle the latest version #1475

Closed
1 of 3 tasks
Tracked by #1601
nicobohne opened this issue Dec 9, 2022 · 15 comments · Fixed by #1486
Closed
1 of 3 tasks
Tracked by #1601

Parcel production builds can't bundle the latest version #1475

nicobohne opened this issue Dec 9, 2022 · 15 comments · Fixed by #1486

Comments

@nicobohne
Copy link

nicobohne commented Dec 9, 2022

Summary

The latest version of Zustand is not compatible with Parcel's builds. The following error is thrown when the bundled code is run:

index.js:41 Uncaught TypeError: Cannot convert undefined or null to object
    at Function.assign (<anonymous>)
    at index.js:41:42

Link to reproduction

https://github.com/nicobohne/zustand-parcel-reproduction

Check List

Please do not ask questions in issues.

  • I understand this is not a question.

Please fill this template if you're filling an issue regarding TypeScript.

  • I've added a link to a typescript playground or codesandbox with a minimal reproduction. Or some other way of reproduction if these two don't suffice.
  • I've read the typescript guide, in particular that create is to be used as create<T>()(...) and not create<T>(...).
@dai-shi
Copy link
Member

dai-shi commented Dec 10, 2022

Thanks for reporting!

Do you have any guess what causes the issue?
Do any previous versions work?

@nicobohne
Copy link
Author

Hey, all previous versions work. I dug a little deeper and found that the problem no longer exists in the latest version when scope hoisting in parcel is disabled (--no-scope-hoist). Also, here is a more detailed screenshot of the error:

screenshot

@dai-shi
Copy link
Member

dai-shi commented Dec 10, 2022

Can you please modify the last line to this and see how it goes?

if (exports.default) { Object.assign(exports.default, exports); module.exports = exports.default; }

@nicobohne
Copy link
Author

nicobohne commented Dec 10, 2022

Okay, manually edited the bundled file and this seems to fix the error.

Edit: Or even directly editing the source file from zustand with the above changes removes the error from production bundles.

@dai-shi
Copy link
Member

dai-shi commented Dec 10, 2022

@barelyhuman Do you have other ideas?

@barelyhuman
Copy link
Collaborator

i could do a empty assign check similar to the above solution but then that's just boiler code I expect bundlers to add.

Let me play around with rollup a bit, i should be able to get a cleaner solution

@barelyhuman
Copy link
Collaborator

barelyhuman commented Dec 10, 2022

@dai-shi

After hacking around, I think if we slowly let people used named imports instead that would be a good solution in the long run.

for now , if I change the index.ts to export both create and a default like it already does, we can provide a solution to #559 and also can mark the default export as deprecated to make it easier for people, since I don't think waiting for each tool to break for an import would be a nice idea? Let me know what you think

export * from './vanilla'
export * from './react'
export { default as createStore } from './vanilla'
import create from './react'
export { create }

/**
 * @deprecated
 */
export default create

@dai-shi
Copy link
Member

dai-shi commented Dec 11, 2022

@barelyhuman

We could add a named export and deprecate the default export for v5, but not sure what @drcmda would think.

What's the issue with the hack? In the long run, I believe tools migrate to native ESM, so this issue will be less likely to happen.

Can you also check what I do in proxy-memoize? Does it help in our case?
https://github.com/dai-shi/proxy-memoize/blob/05c5b873c65d079d953ef9f08833702e29fd2978/package.json#L21

@barelyhuman
Copy link
Collaborator

barelyhuman commented Dec 11, 2022

The node field from what I've read and understood is only read by webpack, could be worth a try since the source of most issues we've had seem to be from webpack users.

No direct issue with using a hack, it's just that we are running in circles with the export thing. We've got enough configuration to compile 10 libraries in the codebase which means more moving parts and harder to control what's going on, plus with the name and direct module.exports we are actually fighting against the other tooling since most of them agree on only having named exports.

Hence, the recommendation to do the slow removal of the default export. I understand the aesthetic value of

import create from "zustand"

so I'd like to make it work in most cases, going throught older React bundling techniques where both the import styled worked.

const React  = require("react");
//and
import React from 'react'

Even when there were almost 6-7 other named exports on the library.
In theory it's just

React = react 
React.Children = Children 

exports = React
exports.default = React 
exports.Children = Children

But I'd like to go through it a little more before I implement it here

@podeig
Copy link

podeig commented Dec 12, 2022

I just want to confirm that I have the same issue with 4.1.5 release. 4.1.4 is ok.

@jaredatron
Copy link

I am still having this issue with [email protected] and [email protected]

@dai-shi
Copy link
Member

dai-shi commented Jan 19, 2023

Do you mean the reproduction in OP is failing with 4.3.2? It should have fixed before.

@nicobohne
Copy link
Author

Hey, I also encountered a new bug in my production build after updating to 4.3.2.

To test this I updated my reproduction to [email protected] and to use named exports (https://github.com/nicobohne/zustand-parcel-reproduction/tree/update/4.3.2). I can confirm that there is a new error.

This time the error is as follows:

Uncaught TypeError: (0 , i.create) is not a function
    at app.js:3:22

I also tested [email protected]. No error occurs in this version. So I guess this error is related to #1531?

Should I open a new issue for this error?

@dai-shi
Copy link
Member

dai-shi commented Jan 21, 2023

I also tested [email protected]. No error occurs in this version. So I guess this error is related to #1531?

Oh, really... Yeah, please open a new issue.

@nicobohne
Copy link
Author

Done: #1563

P.S.: Thx for this fantastic library

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 a pull request may close this issue.

5 participants