-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Angular: Improve Vite compatibility #31686
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
| if ((options as any as StandaloneOptions).enableProdMode) { | ||
| annotations.unshift(require.resolve('./client/preview-prod')); | ||
| annotations.unshift(require.resolve('./client/preview-prod.mjs')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Type assertion to StandaloneOptions could be replaced with proper type narrowing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the Angular preset to load ESM entrypoints (.mjs) instead of CommonJS files for better Vite compatibility.
- Changed
previewAnnotationsimports from CJS to.mjsmodules - Updated production and docs preview entries to reference
.mjsfiles
Comments suppressed due to low confidence (4)
code/frameworks/angular/src/preset.ts:16
- No automated tests appear to cover these new .mjs resolution paths. Consider adding unit tests to verify that previewAnnotations correctly includes the .mjs entrypoints.
export const previewAnnotations: PresetProperty<'previewAnnotations'> = async (
code/frameworks/angular/src/preset.ts:19
- Relying on require.resolve for ESM modules may not work in pure ESM contexts. Consider using Node's createRequire or import.meta.resolve to consistently locate .mjs modules.
const annotations = [...entries, require.resolve('./client/config.mjs')];
code/frameworks/angular/src/preset.ts:22
- Similar to above, using require.resolve for an .mjs production preview file could fail in ESM environments. You might switch to import.meta.resolve or a createRequire workaround.
annotations.unshift(require.resolve('./client/preview-prod.mjs'));
code/frameworks/angular/src/preset.ts:28
- For consistency and reliability in ESM builds, consider using import.meta.resolve or Node's createRequire instead of require.resolve when referencing .mjs files.
annotations.push(require.resolve('./client/docs/config.mjs'));
|
View your CI Pipeline Execution ↗ for commit 8d33332.
☁️ Nx Cloud last updated this comment at |
…eview Angular: Improve Vite compatibility (cherry picked from commit 51ff6d3)
What I did
As I'm investigating a bug report, I noticed that the preset for angular adds the CJS versions of entrypoint files for the builder, which should be ESM.
This has not fully resolved the issue I'm working on, but it does seem to have solved an important issue.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
I'm working with this repro:
https://github.com/eSpoc/vangular-storybook-minimal
And the issue was:
This PR does solve that specific problem, it seems. That error is no longer there.
There's still another Error I need to solve it seems, but that might go in another PR.
The remaining error is:
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake 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 PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
Updates Angular preset configuration to use ESM module extensions for preview annotations, improving Vite compatibility while maintaining webpack5 as the builder.
code/frameworks/angular/src/preset.tsto use.mjsextensions instead of.jsfor preview-related imports