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

NextJS: Fix Image Context re-use via singleton #24146

Merged

Conversation

martinnabhan
Copy link
Contributor

@martinnabhan martinnabhan commented Sep 12, 2023

What I did

#23881 was supposed to fix Image Context reuse, but it turns out that code/frameworks/nextjs/src/preview.tsx uses the CommonJS export (code/frameworks/nextjs/dist/image-context.js), while code/frameworks/nextjs/src/images/next-image.tsx etc. uses the ESM export (code/frameworks/nextjs/dist/image-context.mjs), causing the Image Context to not be reused and thus parameters specified in parameters.nextjs.image and applied in preview.tsx won't be available in next-image.tsx.

This pull request forces the use of the CommonJS export, even when using import, making sure the same Image Context is reused for both ESM and CommonJS imports.

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

If you check the Eager story from the previous pull request you can see the image isn't being eagerly loaded, while the Eager story of this branch properly eager loads the image.

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>

@martinnabhan martinnabhan marked this pull request as ready for review September 12, 2023 06:10
@martinnabhan
Copy link
Contributor Author

@ndelangen Sorry to bother you with this again. Could you please take a look?

@ndelangen ndelangen self-assigned this Sep 18, 2023
@yannbf
Copy link
Member

yannbf commented Sep 19, 2023

Hey @ndelangen would you mind checking this out whenever you have time? Thanks!

It might take some time as we are quite busy, we really appreciate your efforts and patience <3

@ndelangen ndelangen added bug nextjs ci:daily Run the CI jobs that normally run in the daily job. labels Sep 19, 2023
@martinnabhan martinnabhan force-pushed the bugfix/nextjs-image-context-2 branch from 2382868 to bc8a496 Compare October 3, 2023 02:24
@ndelangen ndelangen changed the title NextJS: Fix Image Context reuse (ensure singleton by using the same Image Context for both ESM and CommonJS imports) NextJS: Fix Image Context re-use via singleton Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ci:daily Run the CI jobs that normally run in the daily job. nextjs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants