NextJS: Fix Image Context reuse (ensure singleton by externalizing it)

[martinnabhan]

What I did

I'm not exactly sure when but between #21909 being merged and it being released props specified in parameters.nextjs.image have stopped being applied to next/image.

After some debugging it turns out that the React Context (ImageContext) being used to pass the props from parameters.nextjs.image to next/image isn't being reused, instead it's recreated in every file where it's imported. This happens because webpack inlines the contents of context.ts instead of keeping the import statement intact.

Since we want to reuse the same context everywhere we should make sure webpack preserves the import even after building, which can be done by marking context.ts as an external file that shouldn't be included in the output bundle.

Note we also need to mark decorator.tsx as external, or else it will be included in the preview.js bundle, causing the import of context.ts to fail.

How to test

If you look at the current output you can see ImageContext being recreated in every file it's imported.
For example in dist/preview.js, dist/images/next-image.js or dist/images/next-legacy-image.js you will see the following line:

ImageContext=(0,import_react.createContext)({})

After building this branch the output instead turns into

var import_decorator = require("./images/decorator");

for preview.js and

var import_context = require("./context");

for decorator.js and finally

var import_context = require("./context");

for next-image.js, next-legacy-image.js, and next-future-image.js, which means the context is correctly being reused instead of recreated.

Checklist

• Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
• Make sure to add/update documentation regarding your changes
• If you are deprecating/removing a feature, make sure to update
  MIGRATION.MD

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.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "build", "documentation", "maintenance", "dependencies", "other"]

[ndelangen]

I understand now.

The issue I have is that:

storybook/code/frameworks/nextjs/package.json

Lines 151 to 152 in ba0e4e2

	"./context",
	"./images/decorator",

represents these:

storybook/code/frameworks/nextjs/package.json

Lines 143 to 144 in ba0e4e2

	"./src/images/context.ts",
	"./src/images/decorator.tsx",

But this is not obvious, and it's very fragile to externalize relative paths like that.
Let's say someone decides to rename a file to foobar.tsx to context.tsx, this will suddenly be externalized, and likely break, because that's unexpected, and they won't add an bundler entry for this.

When importing something that's externalized, we can make this very clear:

- import { ImageContext } from './context';
+ import { ImageContext } from '@storybook/nextjs/dist/image/context';

If we do that, there's no relative path to externalize.
In fact, we won't need to declare this as external at all, because the references to itself, are always externalized:

storybook/scripts/prepare/bundle.ts

Lines 61 to 66 in ba0e4e2

	const externals = [
	name,
	...extraExternals,
	...Object.keys(dependencies || {}),
	...Object.keys(peerDependencies || {}),
	];

Importing this way does mean we have to add it to the export map in package.json:

storybook/code/frameworks/nextjs/package.json

Lines 23 to 43 in ba0e4e2

	"exports": {
	".": {
	"types": "./dist/index.d.ts",
	"node": "./dist/index.js",
	"require": "./dist/index.js",
	"import": "./dist/index.mjs"
	},
	"./preset": {
	"types": "./dist/preset.d.ts",
	"require": "./dist/preset.js"
	},
	"./preview.js": {
	"types": "./dist/preview.d.ts",
	"require": "./dist/preview.js",
	"import": "./dist/preview.mjs"
	},
	"./next-image-loader-stub.js": {
	"types": "./dist/next-image-loader-stub.d.ts",
	"require": "./dist/next-image-loader-stub.js",
	"import": "./dist/next-image-loader-stub.mjs"
	},

..or vite will complain.

---

As a general aim, I'd like to have bundler-entries be at the top-level in the src directory.
I know this is not the case everywhere, but if we can do this without too much effort, we should.

So the preferred way would be:

import { ImageContext } from '@storybook/nextjs/dist/image-context';

[martinnabhan]

Thank you for the thorough explanation!

I think I've made the correct changes but now I'm getting this error:
https://app.circleci.com/pipelines/github/storybookjs/storybook/58413/workflows/8333bfd7-9b78-427d-acde-9a05067d485f/jobs/573393?invite=true#step-103-35143_111

Should I just add @ts-expect-error or is there another way of solving this?