Breaks in pnpm environment

[waspeer]

Hi!

Thanks for making this package! It's fantastic :)
I'm using pnpm as my package manager and I have been getting errors after building Sanity studio. To reproduce:

pnpm create sanity@latest

# project: test
# dataset: test
# output: test
# template: no schemas
# typescript: yes

cd test
pnpm install @sanity-typed/types

// sanity.config.ts
import {defineConfig} from '@sanity-typed/types' // <-- change this line to import from sanity-typed
// ...

pnpm run build
pnpm run start

The error I'm getting:

Uncaught error: Pe.default.div is not a function
http://localhost:3333/static/sanity-9dde895e.js:4670:40453
TypeError: Pe.default.div is not a function
    at http://localhost:3333/static/sanity-9dde895e.js:4670:40453
    at http://localhost:3333/static/sanity-9dde895e.js:7423:54634

This error resolves when I change this line back:

// sanity.config.ts
import {defineConfig} from 'sanity' // <-- change this line to import from sanity
// ...

I wasn't able to create a StackBlitz or something similar doing these steps, so I'm not sure how this reproduces on other machines. I'm using a Macbook Pro with Ventura 13.4

[saiichihashimoto]

So I've never used pnpm and I wasn't really planning on supporting it, although it's nice to identify this issue twice as a pnpm specific issue. I have no idea what's happening. My immediate guess is that the default.div error has something to do with styled-components, which I think that sanity uses and, since sanity is a peer dependency, the issue has something to do with how pnpm handles or doesn't handle peer dependencies. Maybe you can help solve this.

Even if I were to support this, I'm not sure how to maintain it. It seems difficult to get CI to setup a repo with each package as a dependency, attempt npm/yarn/pnpm install, build, and determine that running also works (because in this case, it's actually running the studio that identifies the issue). Without this full pipeline, the chances that I re-break pnpm without knowing it are extremely high.

[shixish]

Got snagged by this as well 😢. It'd be nice to add a warning to the readme page saying it'll break if you use pnpm. Sanity offers pnpm as a supported option when you spin up a new project so it's likely that more people are going to stumble here. It's also tricky to figure out what's going on since it works in dev mode but breaks in prod mode. 🤦

[waspeer]

Yeah its a tricky issue. I did not have the time to dig into it yet, but my suspicion is that it has something to do with the portabletext editor dependency, since that is the only one in this package that depends on styled components. I hope to be able to look into this later, but if you find out something I'm curious to hear your findings.

[mrr11k]

Also stumbled on it. 😕 Will also try to dig into it soon.
Good to know that the portabletext editor could be the issue. I will report here if I find something.

[saiichihashimoto]

Anyone have any ideas of how to test this in CI, before we tackle solving it? I took a first swing at it, but ended up with a solution on how to build this whole repo using pnpm, which is not what I want. I want to figure out if, when another repo uses @sanity-typed/types (as an example, we can run tests for each package) and pnpm, that we don't get build errors. I'm also concerned that, even with the reported errors, it's not building the sanity studio that's failing but running it, which seems even worse to test. I'm convinced this isn't just difficult to test, but flaky to maintain.

Basically, how do other libraries that don't use pnpm internally validate when an application using the library does use pnpm?

[saiichihashimoto]

(separately it looks like there's some other issue involving portable text and pnpm which, if we had this CI setup, would hopefully identify that into a separate issue.)

I'm sure pnpm has it's own pros and cons, but the fact that it doesn't work with libraries that do work with npm is pretty terrible. Pretty difficult to argue it's a drop-in replacement for npm when arbitrary libraries just don't work with it.

[waspeer]

Hm yes, it sounds like this will be a very tricky issue to test. Not impossible, but I'm wondering if it's worth the resources and I agree it sounds flaky as hell. Locally I remember testing this by changing stuff, re-running a one line command that built and ran the production app (pnpm run build && pnpm run preview) and then refreshing the browser to see if it worked (probably similar to how you are doing it as well). Also, I remember that Astro had issues with pnpm in the early days as well, maybe they have managed to create some tests for that?

Maybe it's good to first figure out what's causing this issue and then considering if that's something worth adding tests for. What do you think? I'm expecting this to be an upstream bug from either the portable text package, possibly some edge case in how the dependencies are declared that pnpm is stricter about. If we're really unlucky maybe even a bug in pnpm itself.

I agree with your frustration about pnpm. Even though I've ran into annoying issues like these a couple of times now, I still prefer using it myself. Additionally I think it's only growing in popularity, so it's probably worth the effort to figure this out.

[saiichihashimoto]

1. I don't think you're disagreeing, but this library will build using npm. So I'd need to test that the completely npm build library works in a pnpm environment, which isn't as simple as switching everything over to pnpm and seeing if things work. For me, that's most of the complexity. ie, supporting pnpm is very different than having this library just use npm. Likely, similar issues might occur with yarn. Reproducing the environment required for this issue is non-trivial locally or in CI. And, if it isn't reproduced in CI, I can't claim support for it.
2. I'd like to isolate this specific issue about portable text so I can test for it. However, it only surfaces when someone is using pnpm, so I can't reproduce the issue without completing point number 1 anyways (it very well might be a pnpm exclusive problem, which it seems to be). And to include tests that recreate the issue will be difficult because the issue doesn't occur currently, only when in a pnpm environment which requires point number 1.
3. In terms of documenting this, I'm also not documenting whether or not it works in yarn for all the same reasons all packages aren't listing if they work in npm, yarn, pnpm, mynextnpmimplementation, etc. They are supposed to support all npmjs.org packages out of the box, as-is. Even if we can repro this properly, it feels like the maintenance burden is on the wrong side of the fence here.

Maybe this will help? I couldn't really sort it out but it might be helpful.

[waspeer]

I think it's safe to assume this package should work with any popular package manager. I personally don't feel testing for these kind of issues is something you have the responsibility to add to the CI of this package, even though I do understand you can't fully claim to support pnpm if you don't. As also mentioned on the pnpm website, this probably points towards an issue with one of your dependencies and should be fixed there.

I've played around with it a bit more and I'm starting to think the problem is actually the sanity package. This was my process:

1. I cloned this repository and built only the types package using npm
2. I created a clean Sanity studio project with pnpm and linked the local types package to it (pnpm add ../sanity-typed/packages/types)
3. I ran into some issues that seemed to have to do with the built being in CJS and Vite being ESM (I think it was something like module is not defined). I'm not sure how this is different from using the package from the npm registry 🤷‍♂️
4. I built the types package to ESM with tsup
5. Now there were no more runtime issues in the Sanity Studio repository when running the production build, it ran fine, which is interesting on itself. I did get a warning in the console similar to this, this gave me the impression that there's still something weird happening with running styled components unnecessarily. This warning went away if I changed imports back from @sanity-typed/types to sanity.
6. I stubbed out the imports from sanity in the types package (defineArrayMemberNative, defineConfigNative, etc.). I did this by copying the bare necessary code from the Sanity repository into a local file and adding some any's to remove any deeper imports.
7. Now the styled components warning disappeared.

Of course this isn't conclusive at all, especially since I had to build the package to ESM, but I hope I might trigger some new ideas by sharing this.

[saiichihashimoto]

@waspeer, can you push a minimal repro of the sanity project with issues (step 3) and link it here? I'd like to see what is actually happening for myself.

[waspeer]

https://github.com/waspeer/sanity-typed-issue

This repo includes step 1-4 in my comment. Let me know if this works for you.

[saiichihashimoto]

Looks very thorough! I'll take a look soon.

[saiichihashimoto]

This repro is doing too much to actually decipher what is happening.

Trying to rebuild a local version of @sanity-typed/types is creating it's own issues, for example. Uncaught error: The requested module '/@fs/Users/wannes/Projecten/Web/waspeer/sanity-typed-issue/sanity-typed/types/dist/index.js?t=1701806909825' does not provide an export named 'defineArrayMember' is a whole different error because you're attempting to rebuild it on your own. And then linking @sanity-typed/types to your local folder is bound to have (p)npm install do different things. Both are pretty different issues than the original one of Uncaught error: Pe.default.div is not a function.

Can you really slim this down to the error that created this issue, while using @sanity-typed/types how a normal end user would use it?:

[waspeer]

I think we might be talking past each other here. In my last comments I was just sharing my process debugging the issue. I shared my process because this a hard issue to debug and I was hoping we might help each other navigate this that way. The step you wanted me to replicate in the repo was just an intermediate step in this process and not an attempt to replicate the exact original error.

Just to be completely clear, I'm aware there are a few imperfections in my approach, specifically that the local building and linking of the package is not representative of the actual usage of this package. I think the main point is that I got Styled Components related issues that I was able to pinpoint to the importing of the sanity package. The errors were not identical to the one in my first comment, but I do think they're related. I included the CJS/ESM stuff because I thought it was interesting that all the issues disappeared when I build the package to ESM.

[saiichihashimoto]

I think a lot of issues (and solutions) are getting introduced all together under "pnpm issues" and I really can't approach it all as one chunk. But yes, I did miss the point of what you were doing. It also just dawned on me that you didn't have everything in one branch, but in multiple different commits for the different stages. 🤣

This is the order I'd love to accomplish things:

1. A minimal repro repo of an issue that only applies with pnpm.
2. Involving that in the testing so that CI breaks, specifically because of that issue.
3. Getting a fix in.

Jumping to fix 3 is going to be a ticking time bomb for a regression, so I'm trying to defer the rest until we have 1 & 2. I will end up breaking pnpm users without it in CI, so fixing it now without CI testing for it isn't something I think we should do.

[saiichihashimoto]

I was about to try a solution involving npm pack and checking out a separate repo to test importing this, but I'm realizing I can't really even test the specific error that's occuring:

pnpm run build
pnpm run start

The error I'm getting:

Uncaught error: Pe.default.div is not a function
http://localhost:3333/static/sanity-9dde895e.js:4670:40453
TypeError: Pe.default.div is not a function
    at http://localhost:3333/static/sanity-9dde895e.js:4670:40453
    at http://localhost:3333/static/sanity-9dde895e.js:7423:54634

I have no idea how I'd even run start in CI, detect the error, and close the whole thing down with massive flakiness. puppeteer? npm pack, creating a separate repo to import it, installing with pnpm (and I'd probably do yarn too, at that point), building and starting, detecting runtime errors... I think there's a lot of room for flakiness.

I'm just going to run your steps and see if there's an easy fix for this current issue at hand and, if so, release that. But I don't really feel comfortable claiming pnpm support going forward without being able to validate it automatically.

[saiichihashimoto]

OK, I have a super small repro:

https://gist.github.com/saiichihashimoto/1349697a7dbc64096a89ffc7c6cee771

It only fails on pnpm run start, not pnpm run dev, which is interesting.