Fix: vite dev and yarn pnp + vite by unreverting and adding lodash files to optimizeDeps.include

[ndelangen]

Closes #21523 (comment) #21548

Related: #21528, #20443, #21527

What I did

I added the wrapForPnP code back and added the extra optimizeDeps.include entries

How to test

yarn start should work

[IanVS]

I think the core issue here is that vite does not consider absolute paths to be dependencies. So it does not convert them from commonjs to esm.

I worry that the yarn pnp changes here of turning many things into absolute paths will break other dependencies as well. Ideally I would like to remove most or all of the optimizeDeps.include, and not make the array bigger.

I'll try to look into it more soon, but wanted to say that I'm concerned about this change.

[ndelangen]

@IanVS well, absolute paths are needed to have pnp support fully.. What should we do here?

As-is.. we have a regression where the react-vite framework does not support pnp.

This PR fixes that.

[IanVS]

absolute paths are needed to have pnp support fully

Is this because of the architectural design of Storybook, whereby we pass strings around from one package to another to be imported? @arcanis have you encountered other similar projects that have added yarn pnp support (particularly complex projects like ours)? What ways have they approached it? Is it expected that we should have to resolve node_module imports into absolute paths? If so, do you know what those projects have done to support vite, which will not treat absolute paths as dependencies? Any guidance you can give would be much appreciated here.

@ndelangen I'm having a hard time seeing how the changes here would impact the imports of lodash functions, do you have any idea why those changed?

[ndelangen]

Yes that's the reason @IanVS

[ndelangen]

I am also not sure how the lodash issues popped up after this change.

[ndelangen]

@IanVS though it does seem correct that these paths should end with the extention, since this is what we have in the .mjs dist output..

[IanVS]

I agree, but what confuses me is why Vite isn't handling it automatically like every other dep. I'll go ahead and approve so we can move forward, but when I have some time I want to figure out what's really going on, and maybe we can talk about ways that we can try to avoid absolute imports in the future.

[IanVS]

I'm confused why wrapForPnP needs to be defined locally in each file, but I guess it has something to do with the monkeypatching that pnp.cjs is doing to require.

I do like that this gives us a clear string to grep for if we ever find another way to handle these.

[ndelangen]

The require.resolve MUST happen in the file from which the require should be relative to.

the wrapForPnP must be in each file, because it calls require.resolve, and for convenience, it also adds and removes /package.json to the path.