-
Notifications
You must be signed in to change notification settings - Fork 883
feat(preview-server): bundle to esm 💫 #2733
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
🦋 Changeset detectedLatest commit: 7c00644 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
8faa21c to
4115dcf
Compare
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.
4 issues found across 7 files
Prompt for AI agents (all 4 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/preview-server/src/utils/run-bundled-code.spec.ts">
<violation number="1" location="packages/preview-server/src/utils/run-bundled-code.spec.ts:6">
P2: Rule violated: **No `should` in tests**
Test description uses 'should'. Use declarative language instead: 'runs the bundled code in a VM context'.</violation>
<violation number="2" location="packages/preview-server/src/utils/run-bundled-code.spec.ts:21">
P2: Rule violated: **No `should` in tests**
Test description uses 'should'. Use declarative language instead: 'returns an error if the code throws'.</violation>
</file>
<file name="packages/preview-server/src/utils/get-email-component.ts">
<violation number="1" location="packages/preview-server/src/utils/get-email-component.ts:86">
P1: The `context` object is spread/copied into the VM context during `runBundledCode`. Later modifications to `context.shouldIncludeSourceReference` in `renderWithReferences` won't be visible inside the VM, breaking the source reference feature. Consider either returning the contextified object from `runBundledCode` so it can be modified directly, or using a different mechanism to communicate with the VM at render time.</violation>
</file>
<file name="packages/preview-server/src/utils/run-bundled-code.ts">
<violation number="1" location="packages/preview-server/src/utils/run-bundled-code.ts:37">
P2: `import.meta.resolve` is module-scoped and will resolve relative specifiers from `run-bundled-code.ts` rather than from `filename`. Consider wrapping it to use the correct parent URL.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
5f40254 to
19cff15
Compare
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
ba9b64b to
d64215c
Compare
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.
2 issues found across 9 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/react-email/dev/index.js">
<violation number="1" location="packages/react-email/dev/index.js:16">
P2: Setting `NODE_OPTIONS` overwrites any existing value from the user's environment. Consider appending to preserve user-defined Node options.</violation>
</file>
<file name="packages/react-email/src/index.ts">
<violation number="1" location="packages/react-email/src/index.ts:1">
P1: This shebang won't work correctly on most Unix systems. Without the `-S` flag, `env` treats everything after the program name as a single argument. Use `#!/usr/bin/env -S node --experimental-vm-modules --disable-warning=ExperimentalWarning` to properly split the arguments. The `-S` flag is supported by GNU coreutils 8.30+ (2018) and most modern BSD/macOS systems.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
b2a7839 to
97578ae
Compare
I hate CJS, and we needed top-level
awaitfor better integrations, so the stars have aligned for this.To actually make this happen, we're just switching esbuild's options from
format: 'cjs'toformat: 'esm'and now using vm's Module support through the--experimental-vm-modules. The main bit of documentation for what we're using here can be found in https://nodejs.org/api/vm.html#class-vmsourcetextmoduleSummary by cubic
Switch preview-server bundling from CommonJS to ESM to enable top‑level await and modern import semantics. Uses Node’s vm SourceTextModule with experimental VM modules and updates scripts to set NODE_OPTIONS.
New Features
Migration
Written for commit 7c00644. Summary will update on new commits.