CSF-Tools: Fix export specifier bug

[valentinpalkovic]

Closes N/A

What I did

CSF-Tools was not able to analyze aliased named exports properly. An export like the following lead to an Error:

var preview_default = {};

export { preview_default as default };

The fix now makes sure that the local name of the ExportSpecifier is used to find the proper declaration

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 pull request has been released as version 0.0.0-pr-27418-sha-ab9c6633. Try it out in a new sandbox by running npx [email protected] sandbox or in an existing project with npx [email protected] upgrade.

More information

Published version	0.0.0-pr-27418-sha-ab9c6633
Triggered by	@valentinpalkovic
Repository	storybookjs/storybook
Branch	valentin/fix-aliased-named-export-analysis
Commit	ab9c6633
Datetime	Wed May 29 19:51:16 UTC 2024 (1717012276)
Workflow run	9292245868

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=27418

[shilman]

Makes sense. Please add a test case! 🙏

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit dcf4a25. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

---

✅ Successfully ran 2 targets

• nx affected -t check -c production --parallel=7
• nx run-many -t build -c production --parallel=3

---

Sent with 💌 from NxCloud.

[valentinpalkovic]

@shilman Done!

[daneah]

I'm still facing what I think might be a related bug in our project when using 8.2.0-alpha.4—the only information given in the error is the following, so I'm having a pretty hard time being sure or attempting to make a small reproduction. Given you've got some context here, do you think this is related?

transforming (1) iframe.htmlTypeError: Cannot read properties of null (reading 'init')
    at ./node_modules/@storybook/csf-tools/dist/index.js:17:3419
    at Array.forEach (<anonymous>)
    at enter (./node_modules/@storybook/csf-tools/dist/index.js:17:3176)
    at NodePath._call (./node_modules/@babel/traverse/lib/path/context.js:46:20)
    at NodePath.call (./node_modules/@babel/traverse/lib/path/context.js:36:17)
    at NodePath.visit (./node_modules/@babel/traverse/lib/path/context.js:82:31)
    at TraversalContext.visitQueue (./node_modules/@babel/traverse/lib/context.js:89:16)
    at TraversalContext.visitMultiple (./node_modules/@babel/traverse/lib/context.js:61:17)
    at TraversalContext.visit (./node_modules/@babel/traverse/lib/context.js:110:19)
    at traverseNode (./node_modules/@babel/traverse/lib/traverse-node.js:22:17)

[valentinpalkovic]

Hi @daneah

Could be related! Could you please send me the compiled code of node_modules/@storybook/csf-tools/dist/index.js, because unfortunately I can't look up line 17 and column 3419. The line number doesn't match with the version of mine.

[daneah]

@valentinpalkovic Thanks a bunch for taking a look! I've got it up on a pastebin here as I didn't feel pasting it directly here would be particularly helpful. Let me know if I can send it some other way, or do anything else to help out.

[valentinpalkovic]

It seems to be related! Could you please replace the node_modules/@storybook/csf-tools/dist/index.js file by this one?: https://pastebin.com/nv6iPCvk What I did is to place a console.log(self._code) to see which kind of code cannot be parsed. And can you send me the console.log output over?

[daneah]

Awesome, that's sort of what I was thinking of doing once I got a little more time to read what was going on in this module. Here's the code in question, output from that log:

import { setCustomElementsManifest } from '@storybook/web-components';

import customElementsManifest from '../../packages/pharos/custom-elements.json';
import '../styleConfig';
import '../initComponents';

setCustomElementsManifest(customElementsManifest);

export { preview } from '../preview';

If I understand correctly, the issue is with the Babel parsing of this code here directly, and the change you made here improved things around exports, so maybe it's the re-export of preview here that's the bit jamming things up?

[valentinpalkovic]

The issue is that csf-tools doesn't follow imports or re-exports. Therefore, this preview has to define the preview object statically. I understand that this leads to some duplication in your case, but unfortunately, it's impossible to have a global preview.js and then import and re-export it in other preview.js files. That's a limitation of how csf-tools parses preview files.

Correct @shilman?

If you don't want to duplicate code, I would advise writing a small script that gets executed on, for example, a "pre-commit" hook, which copies over the contents of the generic preview.js and takes template framework-specific preview.js files and generates them as soon as the generic preview.js changes.

[daneah]

Thanks for the details—FWIW this worked up until the 8.1.X release, so this is news. I think that seems reasonable and will fix it up on our end, but others might also be surprised at the breakage!

EDIT: I confirm that making the preview statically defined (well, it seems to allow nested values from imports) gets things working again.

EDIT: It seems I can also do the following rather than the reexport:

import { preview as basePreview } from '../preview';

export const preview = basePreview;

[shilman]

@daneah I think you've found a bug. What's happening:

Storybook loads preview.js at runtime (in the browser)

This case should work for any valid JavaScript. So importing & re-exporting, functions that generate stuff, etc. are all "supported" (we don't actually support these things, but they won't break).

Storybook does static analysis on preview.js to determine sort order and project-level tags (in Node)

This case has all kinds of restrictions on what we can statically analyze. The old sort order code is probably wrapped with a try/catch and displays some kind of warning in the console that what you're doing is unsupported. The new tags code introduced in 8.1.0 is not wrapped.

[daneah]

Thanks for looking into this and the quick turnaround @shilman. I understand that your change in the PR ends up being to fix the break in backward compatibility, but that we're still doing something that isn't expressly supported and should continue toward the static definitions that @valentinpalkovic suggested. Am I thinking about that right? Just want to make sure our team proceeds in the best long-term way possible 😄

[valentinpalkovic]

I would recommend having a statically analyzable preview.js file, because in some particular scenarios (as for tags) we don't evaluate but rather parse it. That's definitely more future proof than your current solution :)