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

Addon-docs: Resolve mdx-react-shim & @storybook/global correctly #23941

Merged
merged 8 commits into from
Aug 25, 2023

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Aug 24, 2023

Closes #23217

What I did

I added a require.resolve, which should cause the issue to no longer happen.

When manually testing I found that it was also failing to load @storybook/global.
So I found a solution for that too.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

  1. run yarn task --task run-registry --no-link
  2. wait for it to have the local registry running successfully
  3. unzip the repro
  4. navigate to the unzipped directory
  5. change the pnpm config to use the local registry by making a change to the .npmrc-file:
    + registry=http://localhost:6001/
  6. prune the pnpm store cache (pnpm store prune)
  7. remove node_modules (there are also 2 in the packages)
  8. run the install (pnpm install)
  9. navigate to the ./packages/storybook directory
  10. Run yarn build
  11. Expect it to work.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
  • Make sure this PR contains one of the labels below:

🦋 Canary release

This pull request has been released as version 0.0.0-pr-23941-sha-8f1ed079. Install it by pinning all your Storybook dependencies to that version.

More information
Published version 0.0.0-pr-23941-sha-8f1ed079
Triggered by @ndelangen
Repository storybookjs/storybook
Branch norbert/fix-23217
Commit 8f1ed079
Datetime Fri Aug 25 12:43:08 UTC 2023 (1692967388)
Workflow run 5976017700

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=23941

@yannbf yannbf changed the title Docs-Addon: Fix #23217 Addon-docs: Resolve mdx-react-shim correctly Aug 24, 2023
@ndelangen ndelangen changed the title Addon-docs: Resolve mdx-react-shim correctly Addon-docs: Resolve mdx-react-shim correctly Aug 24, 2023
@IanVS
Copy link
Member

IanVS commented Aug 24, 2023

Note the user in the issue report is using pnpm. Could the problem be that we are not exposing the shim correctly? Also, by using require.resolve, does that prevent us from shipping ESM?

@ndelangen
Copy link
Member Author

@IanVS that's a great comment, I made a change so it should be able to get the .mjs-file.

@ndelangen ndelangen changed the title Addon-docs: Resolve mdx-react-shim correctly Addon-docs: Resolve mdx-react-shim & @storybook/global correctly Aug 25, 2023
@storybook-bot
Copy link
Contributor

Failed to publish canary version of this pull request, triggered by @IanVS. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/5975903351

@IanVS
Copy link
Member

IanVS commented Aug 25, 2023

Oh, I thought I was a core team member, so I tried to build a canary to test this out in my own pnpm project, but I don't have permissions. @ndelangen maybe you can trigger a canary?

@ndelangen
Copy link
Member Author

@ndelangen ndelangen added the pnpm label Aug 25, 2023
Copy link
Member

@IanVS IanVS left a comment

Choose a reason for hiding this comment

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

Thanks @ndelangen. I tested out the canary in both my own non-monorepo pnpm react-vite storybook project (which was not failing previously), as well as in the monorepo pnpm react-webpack reproduction provided in the issue, both of which worked fine.

I think eventually, if we want to support true node.js ESM, we'll need to start using createRequire, since our .mjs files that are generated right now will throw errors if require is not available, but that can be a matter for another day.

config.mdxCompileOptions.providerImportSource = '@storybook/addon-essentials/docs/mdx-react-shim';
config.mdxCompileOptions.providerImportSource = join(
dirname(require.resolve('@storybook/addon-docs/package.json')),
'/dist/shims/mdx-react-shim'
Copy link
Member

@IanVS IanVS Aug 25, 2023

Choose a reason for hiding this comment

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

Why do we need to use /dist/shims here, when the exports of addon-docs includes mdx-react-shim ? I thought the way we normally handle bundlers that don't support exports (webpack and vite both do, though) was by putting a file at the root which re-exported from dist. That feels like a cleaner solution to me, rather than sprinkling dist throughout our source.

Copy link
Member Author

Choose a reason for hiding this comment

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

@IanVS right, I'd definitely not want users to import directly from dist, because what's in dist is internal for us.
But that's also the reaosn why I think here it's OK to use:
This is all contained in the monorepo, so I'm less afraid this will break.

Sure I could direct to a root file, and then the root file directs to dist.. but I'd rather have less hoops and less files that need to be there for obscure reasons.

If this works, and therefore 1 less reason for having an root files that's 👍 for me.

I'm hoping that long term we won't have any root files anymore.. but that's likely going to be around the same time when:

we want to support true node.js ESM, we'll need to start using createRequire, since our .mjs files that are generated right now will throw errors if require is not available, but that can be a matter for another day.

@ndelangen ndelangen merged commit 67d84c8 into next Aug 25, 2023
23 of 24 checks passed
@ndelangen ndelangen deleted the norbert/fix-23217 branch August 25, 2023 14:07
@github-actions github-actions bot mentioned this pull request Aug 25, 2023
15 tasks
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.

[Bug]: Module not found: Error: Can't resolve @storybook/addon-docs/mdx-react-shim
3 participants