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

Fix output format of @web/dev-server-storybook #2407

Merged
merged 7 commits into from
Aug 4, 2023

Conversation

koddsson
Copy link
Contributor

@koddsson koddsson commented Aug 4, 2023

Set moduleResolution to node16 so that TypeScript will output ES Modules and changed the module property so that it's not a moving target.

Fixes #2405

@changeset-bot
Copy link

changeset-bot bot commented Aug 4, 2023

🦋 Changeset detected

Latest commit: 4dfc0fc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@web/dev-server-storybook Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@koddsson koddsson changed the title Try fixing es module Fix output format of @web/dev-server-storybook Aug 4, 2023
@koddsson koddsson marked this pull request as ready for review August 4, 2023 08:10
@koddsson koddsson merged commit e85651b into modernweb-dev:master Aug 4, 2023
5 checks passed
@koddsson koddsson deleted the try-fixing-es-module branch August 4, 2023 11:51
@@ -1,7 +1,8 @@
import mdx from '@mdx-js/mdx';
import { transformAsync } from '@babel/core';
// @ts-ignore
import { createCompiler } from '@storybook/csf-tools/mdx';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get this error:

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/Users/westbrookjohnson/Documents/repos/spectrum-web-components/node_modules/@storybook/csf-tools/mdx' imported from /Users/westbrookjohnson/Documents/repos/spectrum-web-components/node_modules/@web/dev-server-storybook/dist/shared/mdx/transformMdxToCsf.js
Did you mean to import @storybook/csf-tools/mdx.js?
    at new NodeError (node:internal/errors:387:5)
    at finalizeResolution (node:internal/modules/esm/resolve:429:11)
    at moduleResolve (node:internal/modules/esm/resolve:1006:10)
    at defaultResolve (node:internal/modules/esm/resolve:1214:11)
    at nextResolve (node:internal/modules/esm/loader:165:28)
    at ESMLoader.resolve (node:internal/modules/esm/loader:844:30)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:431:18)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:76:40)
    at link (node:internal/modules/esm/module_job:75:36) {
  code: 'ERR_MODULE_NOT_FOUND'
}

Goes away if I add .js locally, but then I get a different error.

Copy link
Member

@bashmish bashmish Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm this

the 2nd error in chain is

/path/to/project/node_modules/@web/dev-server-storybook/dist/build/rollup/createRollupConfig.js:4
import html from '@web/rollup-plugin-html';
       ^^^^
SyntaxError: The requested module '@web/rollup-plugin-html' does not provide an export named 'default'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:124:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:190:5)

@koddsson did you test this change with some storybook?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't manually test this no. I was kind of relying on the tests to catch this. I find it weird that they didn't 🤔

I haven't had a lot of time to look into this but I am trying to fix this today. I've gotten to a point where I don't get any errors in a local project but I am just getting a empty page so I'm looking further.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@koddsson are there integration tests that run a real storybook in the test environment or smth like that? if so, maybe good to improve that to detect such cases
but I thought we didn't have them, maybe some unit tests only or so, but I didn't look into that yet
for the Storybook v7 migration I think we need to improve a lot in this space

Copy link
Contributor Author

@koddsson koddsson Aug 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I got this fixed now. It's at least working in my local project and the tests are all running green locally. Could you take a look at #2415 @Westbrook and @bashmish ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[dev-server-storybook] v1 published files are wrong format
3 participants