[Bug]: handlebars encodes = character in patched pnpm dependency, breaks preview annotations in @storybook/builder-webpack5

[askoufis]

Describe the bug

PNPM injects a hash containing an = to the file path of patched dependencies, this looks something like _patch_hash=xpvpw3nxoacxewbpf6zgrfgd4u_. handlebars encodes this = to =. AFAIK this is an intentional feature of handlebars.js, you can verify this in the handlebars playground by putting an = in the input. This encoded character causes webpack to fail to resolve previewAnnotations that are injected via handlebars. This becomes an issue if you happen to have pnpm patched a dependency that contains a preview annotation.

Relevant code:

storybook/code/builders/builder-webpack5/src/preview/iframe-webpack.config.ts

Lines 133 to 144 in fc3b4b7

	virtualModuleMapping[configEntryPath] = handlebars(
	await readTemplate(
	require.resolve(
	'@storybook/builder-webpack5/templates/virtualModuleModernEntry.js.handlebars'
	)
	),
	{
	storiesFilename,
	previewAnnotations,
	}
	// We need to double escape `\` for webpack. We may have some in windows paths
	).replace(/\\/g, '\\\\');

Maybe you've patched an addon's preview annotation, or in my case we're symlinking the user's .storybook/preview.js file to a storybook config folder controlled by our framework (which users install as a dependency in their project). We do this because we have a pretty involved webpack config that we need to inject into storybook's webpack config, so we maintain control over main.js, while allowing users to provide their own preview.js.

You could argue a few things here:

• PNPM using = in paths isn't great (IMO), but I'm sure there's a reason for it
• handlebars encoding some characters for you makes it unfit for this specific purpose. You already double escape \ anyway, so what's one more tweak to the handlebars output.
• We have a niche use case that's not worth supporting. I agree, but I think supporting pnpm patched dependencies is worth it, even though that's also a niche use case.

To Reproduce

1. Clone https://github.com/askoufis/storybook-repro
2. pnpm install
3. pnpm run storybook. Webpack can't resolve the file, storybook crashes.
4. Check out the previous commit (before patching @storybook/addon-interactions): git checkout 1ac0916b180e505d3c281e8b301027d99a9a30c5
5. pnpm install
6. pnpm run storybook. Storybook works fine.

System

Environment Info:

  System:
    OS: macOS 13.4
    CPU: (10) arm64 Apple M1 Pro
  Binaries:
    Node: 18.16.0 - ~/.local/share/.volta/tools/image/node/18.16.0/bin/node
    Yarn: 1.22.19 - ~/.local/share/.volta/tools/image/yarn/1.22.19/bin/yarn
    npm: 9.5.1 - ~/.local/share/.volta/tools/image/node/18.16.0/bin/npm
    pnpm: 8.6.0 - ~/.local/share/.volta/bin/pnpm
  Browsers:
    Chrome: 113.0.5672.126
    Edge: 113.0.1774.57
    Safari: 16.5
  npmPackages:
    @storybook/addon-essentials: ^7.0.18 => 7.0.18
    @storybook/addon-interactions: ^7.0.18 => 7.0.18
    @storybook/addon-links: ^7.0.18 => 7.0.18
    @storybook/blocks: ^7.0.18 => 7.0.18
    @storybook/react: ^7.0.18 => 7.0.18
    @storybook/react-webpack5: ^7.0.18 => 7.0.18
    @storybook/testing-library: ^0.0.14-next.2 => 0.0.14-next.2

Additional context

No response

[shilman]

Hi @askoufis this is a pretty specific edge case. Any idea what we could change on the Storybook side to fix it?

[askoufis]

Hi @askoufis this is a pretty specific edge case. Any idea what we could change on the Storybook side to fix it?

You're right, it is quite specific. However, handlebars was originally designed for templating HTML, as opposed to JS/TS source code templating, so I think it's worth tweaking how it's used to make it better fit the use case, that is specifically templating file paths.

I think there are a few potential options to fix this:

• Add another replace that un-encodes the = encoding. This is a bandaid fix that only fixes this specific issue, but does nothing to mitigate future encoding issues should they arise.
• Opt-out of HTML escaping by using triple curly braces in the template. This could be dangerous, but potentially viable if we can validate that the input is not malicious (somehow).
• Register a custom handlebars helper that conditionally escapes certain character, or at least validates that the input is a file path or something before not escaping it with Handlebars.SafeString.

Luckily this appears to be the only place where handlebars from @storybook/core-common is used, so there's not much risk in tweaking it to fit the use case.

[temm1210]

How is it going? any update?? facing the same issue

[r34son]

Also facing

[LightScrool]

Was facing the same issue, fixed it with another patch.

Patch command:

pnpm patch @storybook/[email protected]

In file dist/presets/preview-preset.js
Replace this:

.replace(/\\/g,"\\\\")

With that:

.replace(/\\/g,"\\\\").replace("=", "=")

In file dist/index.js
Replace this:

;return presets.apply("webpack",{},{...options,babelOptions,typescriptOptions,frameworkOptions})

With that:

,config = await presets.apply("webpack", {}, {...options, babelOptions, typescriptOptions, frameworkOptions}),obj = config.plugins[0]._staticModules,moduleKey = Object.keys(obj).find(key => key.endsWith('storybook-config-entry.js')),moduleValue = obj[moduleKey],moduleNewValue = moduleValue.replace("=", "=");obj[moduleKey] = moduleNewValue;return config

[askoufis]

After upgrading my reproduction to the latest prerelease version (8.4.0-alpha.6 at time of writing) the bug appears to be fixed. I'm guessing #29208 fixed the issue as handlebars is no longer used within Storybook.

@shilman Should I close this issue even though the fix hasn't be released in a stable version, or should it be kept open until 8.4.0 is released?

[shilman]

Thanks for following up on this @askoufis ! Let's close it as fixed. The fix is available in an alpha today and will be stable in a few weeks.