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

Limit to only using require to let Rollup bundler to detect the file as cjs file #857

Merged
merged 7 commits into from
Mar 26, 2024

Conversation

indigolain
Copy link
Contributor

May fix #856 ?
Having import statements in a file makes @rollup/plugin-commonjs to skip the conversion from CommonJS to ES Module.
Changing the import statement to require should make @rollup/plugin-commonjs handle the file and successfully convert it.

@indigolain indigolain force-pushed the limit-to-only-using-require branch from 87d873d to 2aab9be Compare March 20, 2024 04:56
@indigolain indigolain marked this pull request as draft March 20, 2024 05:14
@indigolain indigolain marked this pull request as ready for review March 20, 2024 05:14
@indigolain indigolain force-pushed the limit-to-only-using-require branch from f4424e4 to 2aab9be Compare March 20, 2024 05:21
@@ -80,6 +80,7 @@ export class StoriesBrowser extends BaseBrowser {
timeout: 60_000,
},
);
await this.page.waitForTimeout(500);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to throw an Execution context was destroyed error if we don't wait for some time here, so I added some timeouts. I'm not sure why this is happening though.
ref: https://github.com/reg-viz/storycap/actions/runs/8354226734/job/22867216673#step:7:99

@en-rai
Copy link

en-rai commented Mar 24, 2024

when will this pr be merged? waiting for this 👍

@Quramy

This comment was marked as resolved.

@indigolain
Copy link
Contributor Author

@Quramy
Thank you for reviewing my PR!
The error you are getting is because the vite project that I setup expects storycap to be installed and present under node_modules ( this is where the config is applied).

Without setting this resolve.alias, when you try to build the storybook, it will result in throwing the following error.

=> Failed to build the preview
Error: [commonjs--resolver] Failed to resolve entry for package "storycap". The package may have incorrect main/module/exports specified in its package.json.

It seems that the commonjs--resolver requires the storycap dependency to be inside package.json by default, unless you configure the resolver.

I've added a pre hooks to build-storybook for populating the storycap build with 5c26134 to resolve the error you are getting, should this resolve the issue?

"build-storybook": "storybook build",
"prestorybook": "../../scripts/e2e-prestorybook.js .",
"prebuild-storybook": "../../scripts/e2e-prestorybook.js .",
"storycap:all": "yarn prestorybook && yarn test && storycap --verbose --server-timeout 100000 --server-cmd \"storybook dev -p 9009\" \"http://localhost:9009\""
Copy link
Member

@Quramy Quramy Mar 26, 2024

Choose a reason for hiding this comment

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

It should be tested with not dev mode but production build such as:

    "storycap:all": "yarn build-storybook && yarn test && storycap --verbose --server-timeout 100000 --server-cmd \"http-server -p 9009 storybook-static\" \"http://127.0.0.1:9009\""

Note

The following script requires yarn add http-server -D

import { ScreenshotOptions } from '../shared/types';
import { triggerScreenshot } from './trigger-screenshot';
import type { ScreenshotOptions } from '../shared/types';
const { triggerScreenshot } = require('./trigger-screenshot');
Copy link
Member

Choose a reason for hiding this comment

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

Is it means that import statements are prohibited under storycap/src/client ? 🤔

@Quramy Quramy merged commit eaf6382 into reg-viz:master Mar 26, 2024
10 checks passed
@indigolain indigolain deleted the limit-to-only-using-require branch March 26, 2024 08:22
@indigolain
Copy link
Contributor Author

Thank you!!

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.

non-legacy withScreenshot decorator stopped working after 4.3.0
3 participants