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

Core: Fix builders/renderer PnP support #21486

Merged
merged 2 commits into from
Mar 9, 2023
Merged

Core: Fix builders/renderer PnP support #21486

merged 2 commits into from
Mar 9, 2023

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Mar 8, 2023

Closes https://discord.com/channels/486522875931656193/1081643102349049937/1081643102349049937

Here's the repro for the bug: https://github.com/ndelangen/storybook-7-vite-test

What I did

We use dirname(require.resolve('${package}/package.json')) in a bunch of places, and actually the bug was that we forgot in som places.

This makes the code look the same everywhere, using the very clear wrapForPnP function (which has to be defined locally!!!).

@ndelangen ndelangen self-assigned this Mar 8, 2023
@ndelangen ndelangen changed the title Norbert/wrap-for-pnp Bug: add resolve wrappers around builders/renderers in presets for PnP Mar 8, 2023
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to:

  • add wrapForPnP to core-common
  • re-export it from each of the frameworks to be used in user main.js

@ndelangen
Copy link
Member Author

Nope, not possible:

(which has to be defined locally!!!)

the require.resolve must happen in the file in the module that depends on the package being resolved.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@shilman shilman changed the title Bug: add resolve wrappers around builders/renderers in presets for PnP Core: Fix builders/renderer PnP support Mar 9, 2023
@shilman shilman added yarn and removed presets labels Mar 9, 2023
@shilman shilman merged commit 99bcb2c into next Mar 9, 2023
@shilman shilman deleted the norbert/wrap-for-pnp branch March 9, 2023 09:25
ndelangen added a commit that referenced this pull request Mar 10, 2023
ndelangen added a commit that referenced this pull request Mar 10, 2023
Revert "Merge pull request #21486 from storybookjs/norbert/wrap-for-pnp"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants