Core: Fix fail to load main.ts error message

[ndelangen]

Closes: #23972
Closes: #25814

What I did

Catch the error in loadMainConfig, and transform the error to be more helpful.

The user was able to do something before https://github.com/storybookjs/storybook/pull/23018/files but after that change, users can have a broken main.ts file simply by importing (with a declarative import) a module that has no commonjs variant.

This was an unnoticed breaking change in SB 7.1

Turning back the change is possible, but very much not preferable, because it would be a performance regression.

I hope that the message will be clear enough for users to act upon it.

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

• Create a sandbox (doesn't matter if linked or not)
• Add "@nuxt/kit": "3.10.1" as a dependency & install
• Add the following to the sandbox's main.ts:

  import { buildNuxt, loadNuxt, tryUseNuxt } from '@nuxt/kit'
  console.log({ buildNuxt, loadNuxt, tryUseNuxt });

• start the sandbox

Before this change the command output looked like:

After this change it should look like:

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-26035-sha-a321a58d. 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-26035-sha-a321a58d
Triggered by	@valentinpalkovic
Repository	storybookjs/storybook
Branch	norbert/handle-esm-main-error
Commit	a321a58d
Datetime	Tue Feb 20 10:53:48 UTC 2024 (1708426428)
Workflow run	7972315424

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

[valentinpalkovic]

I personally haven’t ever heard the term „imperative import“ so far. I think the term „dynamic import" is far more common and is also used on docs sites like https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/import or https://nodejs.org/api/esm.html#import-expressions

[vanessayuenn]

@shilman actually suggested to add a config for this. is that still a viable option (in addition to catching the error)?

[valentinpalkovic]

@ndelangen People might switch to dynamic imports on the top level. And AFAIK, top-level awaits aren't supported since esbuild transforms the main.ts file into a CommonJS one, where top-level awaits don't work. It may make sense to clarify this additionally.

[ndelangen]

@valentinpalkovic yes, hence the " where they are used.`-part.

When they made this change, and run into the "top level await" error, they'll know what to do?

[ndelangen]

@vanessayuenn @shilman where would this config exist?
In the main.ts file that we're failing to load? 😅

The only place I can imagine would be inside a CLI flag.
And I'm not a fan of adding a CLI flag for this.

[ndelangen]

[yannbf]

could this bring potential problems for the core-events package? as chalk would be prebundled in something that goes to the browser as well

[ndelangen]

How, could it get pre-bundled into something that goes into the browser, due to this change?

[ndelangen]

AFAIK this doesn't have an affect on any code we ship to the browser, as long as that browser code doesn't reference chalk.

I had to move chalk into here as @vanessayuenn asked me to move this to core-events: #26035 (comment)

[shilman]

Also have a funny feeling about adding chalk to that package, but I'll leave that decision to you

[shilman]

I'm ok with this workaround rather than making it configurable.

However can we link show an example of a dynamic import in the error message? Do we support top level await or do they need to make async preset functions for that?

[ndelangen]

@shilman do you mean like this:
#26035 (comment)

[ndelangen]

We do not support top-level await:

(norbert/handle-esm-main-error)⚡ [1] % nr storybook                                                 ~/Projects/Storybook/mono/storybook/sandbox/react-vite-default-ts
@storybook/cli v8.0.0-beta.2

Error: Transform failed with 1 error:
./.storybook/main.ts:6:0: ERROR: Top-level await is currently not supported with the "cjs" output format
    at failureErrorWithLog (./node_modules/esbuild/lib/main.js:1649:15)
    at ./node_modules/esbuild/lib/main.js:847:29
    at responseCallbacks.<computed> (./node_modules/esbuild/lib/main.js:703:9)
    at handleIncomingPacket (./node_modules/esbuild/lib/main.js:762:9)
    at Socket.readFromStdout (./node_modules/esbuild/lib/main.js:679:7)
    at Socket.emit (node:events:513:28)
    at addChunk (node:internal/streams/readable:324:12)
    at readableAddChunk (node:internal/streams/readable:297:9)
    at Readable.push (node:internal/streams/readable:234:10)
    at Pipe.onStreamRead (node:internal/stream_base_commons:190:23)

WARN Broken build, fix the error above.
WARN You may need to refresh the browser.

We never have supported it.

[shilman]

Yes but what's the actual replacement code for that example?

const { } = await import('...');

Does that top level await work in main.ts? Can we actually spell it out for users?

[ndelangen]

We do not support top-level await

Spelling it out for users would be extremely complex.
I do not see a way to do that, do you?

[ndelangen]

We're going to add an example of converting from static import to dynamic import..
We'll do that in a comment in the original issue.
We'll link to that comment from the error.

[ndelangen]

Here's what it looks like when there's an evaluation/transform error inside of main.ts:

This is how it looks when there's an static import we do not support:

Here's how a runtime error looks like:

And here's when main.ts cannot be found:

Everything through Error classes in server-errors 🎉

[ndelangen]

Here's the link to the instruction to users:
#23972 (comment)

[shilman]

Also have a funny feeling about adding chalk to that package, but I'll leave that decision to you

[ndelangen]

What's the funny feeling?
Should I just let the full error-body be all-red in the terminal?
Should I use something else?

What are the concerns with this?