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-actions: Fix module resolution for react-native #25296

Merged
merged 3 commits into from Jan 12, 2024

Conversation

dannyhw
Copy link
Member

@dannyhw dannyhw commented Dec 21, 2023

Closes #

What I did

For some reason when the cjs code resolves for react native then @storybook/global cannot be resolved from addon-actions. By adding the react-native field to point to the mjs version the global package can resolve correctly. The alternative is the user adding confusing code to their metro config.

I made this change in my node_modules and it worked without further changes.

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

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

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:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

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

@dannyhw dannyhw added the maintenance User-facing maintenance tasks label Dec 21, 2023
@dannyhw dannyhw requested a review from shilman December 21, 2023 15:23
@dannyhw dannyhw changed the title Fix addon-action module resolution broken for react-native React Native: Fix addon-action module resolution broken for react-native Dec 21, 2023
Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

I don't feel very comfortable with this change, considering it's untestable in the monorepo, and not really knowable what entry/format will work.

It's also unclear if this should be done for other addons, or core packages.

@dannyhw
Copy link
Member Author

dannyhw commented Dec 21, 2023

That makes sense, it is a bit of a weird situation because I don't fully understand why loading the cjs should even cause this.

ESM support in metro is spotty and from my conversations with expo/metro people its not recommend to make changes to resolverMainFields to include module. The suggestion they had was to have a custom resolver, but adding that kind of code to every user project would be complicate the setup a lot.

@ndelangen
Copy link
Member

OK, maybe this is the only way forwards then?

@shilman How do you feel about adding such an entry to each package relevant for react-native?

@dannyhw
Copy link
Member Author

dannyhw commented Jan 3, 2024

I think the problem might actually be something in the way addon-actions is bundled to cjs. Since in theory cjs should actually work better for react native.

The part that I think breaks this is that globals get required with this "dynamic" require.

node_modules/@storybook/addon-actions/dist/index.js

var __require=(x=>typeof require<"u"?require:typeof Proxy<"u"?new Proxy(x,{get:(a,b)=>(typeof require<"u"?require:a)[b]}):x)(function(x){if(typeof require<"u")return require.apply(this,arguments);throw Error('Dynamic require of "'+x+'" is not supported')})

Dynamic requires aren't supported in react native so could be a cause (require.context is available in preview).

Plus it could just use the global that is being included at the start of the file, probably could just do a normal require also?

image image

@shilman shilman changed the title React Native: Fix addon-action module resolution broken for react-native Addon-actions: Fix module resolution for react-native Jan 12, 2024
@shilman shilman merged commit 3abd0ba into next Jan 12, 2024
5 checks passed
@shilman shilman deleted the dannyhw/fix-addon-actions-react-native branch January 12, 2024 11:56
@github-actions github-actions bot mentioned this pull request Jan 12, 2024
27 tasks
@shilman shilman added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Jan 12, 2024
storybook-bot pushed a commit that referenced this pull request Jan 12, 2024
…react-native

Addon-actions: Fix module resolution for react-native
(cherry picked from commit 3abd0ba)
@github-actions github-actions bot mentioned this pull request Jan 12, 2024
6 tasks
storybook-bot pushed a commit that referenced this pull request Jan 12, 2024
…react-native

Addon-actions: Fix module resolution for react-native
(cherry picked from commit 3abd0ba)
@github-actions github-actions bot added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: actions ci:normal maintenance User-facing maintenance tasks patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch react-native
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants