Change compiler ID generation logic to use Node.js import specifier #899
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 0980c5c The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 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 |
Contributor
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests🌍 Community Worlds (169 failed)mongodb (42 failed):
redis (42 failed):
starter (43 failed):
turso (42 failed):
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
✅ 📋 Other
|
Contributor
This was referenced Jan 30, 2026
Member
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Closed
![vercel[bot]](https://avatars-githubusercontent-com.github.com/in/8329?s=60&v=4){kind=small}
Comment on lines
+33
to
+35
| const root = dirname(dir); | ||
|
|
||
| while (dir !== root) { |
Contributor
There was a problem hiding this comment.
The findPackageJson function only checks one parent directory due to incorrect loop termination condition, preventing it from finding package.json files in higher directories.
This was referenced Jan 30, 2026
TooTallNate
force-pushed
the
01-13-add_support_for_use_step_functions_in_class_instance_methods
branch
from
334dd56 to
86c563c
Compare
TooTallNate
force-pushed
the
01-30-change_compiler_id_generation_logic_to_use_node.js_import_specifier
branch
from
6917369 to
0980c5c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request changes the compiler's ID generation logic from using file paths to using Node.js import specifiers. The change enables stable workflow/step/class IDs when the same package specifier resolves to different files depending on package.json export conditions.
Changes:
- Modified ID format from
workflow//path/file.ts//functionNametoworkflow//./path/file//functionNamefor local files andworkflow//packageName@version//functionNamefor packages - Added
module_specifierparameter to SWC plugin config to support package-based ID generation - Implemented module specifier resolution logic in
@workflow/buildersto detect node_modules and workspace packages - Updated all test fixtures to reflect the new ID format with "./" prefix for local files and stripped extensions
Reviewed changes
Copilot reviewed 139 out of 139 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/swc-plugin-workflow/transform/src/naming.rs | Core ID formatting logic, adds module path resolution and extension stripping |
| packages/swc-plugin-workflow/transform/src/lib.rs | Integration of module_specifier parameter into StepTransform |
| packages/swc-plugin-workflow/src/lib.rs | WASM plugin config with new moduleSpecifier option |
| packages/builders/src/module-specifier.ts | New module implementing package detection and import path resolution |
| packages/builders/src/apply-swc-transform.ts | Passes module specifier to SWC transform |
| packages/builders/src/base-builder.ts | Uses package names for imports to respect export conditions |
| packages/rollup/src/index.ts | Integration of module specifier resolution |
| packages/next/src/loader.ts | Integration of module specifier resolution |
| packages/swc-plugin-workflow/spec.md | Updated specification with new ID format and examples |
| Test fixture files (60+ files) | All updated to reflect new ID format |
| packages/core/e2e/e2e.test.ts | Updated regex to match new ID format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+32
to
+33
| let dir = dirname(filePath); | ||
| const root = dirname(dir); |
There was a problem hiding this comment.
The findPackageJson function has a potential infinite loop issue. The termination condition while (dir !== root) where root = dirname(dir) is incorrect. This will cause the loop to never terminate on Unix-like systems where the root is "/", since when dir becomes "/", dirname("/") also returns "/", making dir !== root always true.
The termination condition should check if we've reached the filesystem root by comparing dir with its parent directory, e.g., while (dir !== dirname(dir)).
| use std::path::PathBuf; | ||
| use swc_core::ecma::{ | ||
| transforms::testing::{FixtureTestConfig, test_fixture}, | ||
| transforms::testing::{test_fixture, FixtureTestConfig}, |
There was a problem hiding this comment.
The imports in this file are being reordered. While this doesn't affect functionality, it's a formatting change that should ideally be done separately or avoided if the project doesn't enforce import ordering. The change from FixtureTestConfig, test_fixture to test_fixture, FixtureTestConfig appears to be an alphabetical sorting, but mixing this with functional changes makes the diff harder to review.
Suggested change
| transforms::testing::{test_fixture, FixtureTestConfig}, | |
| transforms::testing::{FixtureTestConfig, test_fixture}, |
| use std::path::PathBuf; | ||
| use swc_core::ecma::{ | ||
| transforms::testing::{FixtureTestConfig, test_fixture}, | ||
| transforms::testing::{test_fixture, FixtureTestConfig}, |
There was a problem hiding this comment.
The imports in this file are being reordered. While this doesn't affect functionality, it's a formatting change that should ideally be done separately or avoided if the project doesn't enforce import ordering. The change from FixtureTestConfig, test_fixture to test_fixture, FixtureTestConfig appears to be an alphabetical sorting, but mixing this with functional changes makes the diff harder to review.
Suggested change
| transforms::testing::{test_fixture, FixtureTestConfig}, | |
| transforms::testing::{FixtureTestConfig, test_fixture}, |
Comment on lines
+88
to
+100
| const rootPkgPath = join(projectRoot, 'package.json'); | ||
|
|
||
| // Walk up to find the package.json directory | ||
| let dir = dirname(filePath); | ||
| while (dir !== dirname(dir)) { | ||
| const pkgPath = join(dir, 'package.json'); | ||
| if (existsSync(pkgPath)) { | ||
| // If this is the root package.json, it's not a workspace package | ||
| if (pkgPath === rootPkgPath) { | ||
| return false; | ||
| } | ||
| // Found a package.json that's not the root - it's a workspace package | ||
| return true; |
There was a problem hiding this comment.
The path comparison if (pkgPath === rootPkgPath) may not work correctly on Windows due to path separator differences and case sensitivity. The rootPkgPath is created using join(projectRoot, 'package.json'), while pkgPath is created from dir which was derived from filePath. If these paths use different separators (backslashes vs forward slashes) or have different casing, the comparison will fail even when they refer to the same file.
Consider normalizing both paths before comparison, for example:
const normalizedPkgPath = pkgPath.replace(/\\/g, '/').toLowerCase();
const normalizedRootPkgPath = rootPkgPath.replace(/\\/g, '/').toLowerCase();
if (normalizedPkgPath === normalizedRootPkgPath) {Alternatively, use path.resolve() on both paths to ensure they're in the same format.
| "@workflow/next": patch | ||
| --- | ||
|
|
||
| Change compiler ID generation logic to use Node.js import specifier |
There was a problem hiding this comment.
The changeset description "Change compiler ID generation logic to use Node.js import specifier" is somewhat vague and doesn't fully explain the impact of this change. Consider expanding it to mention:
- This is a breaking change that alters the format of workflow/step/class IDs
- IDs now use "./path/to/file" format instead of "path/to/file.ext" for local files
- Package files now use "packageName@version" format
- This enables stable IDs across different package.json export conditions
This will help users understand the impact when reviewing changelogs.
Suggested change
| Change compiler ID generation logic to use Node.js import specifier | |
| Change compiler ID generation logic to use Node.js import specifiers and update ID formats. | |
| This is a breaking change that alters the format of workflow, step, and class IDs: | |
| - Local files now use a "./path/to/file" format instead of "path/to/file.ext". | |
| - Package files now use a "packageName@version" format. | |
| - This enables stable IDs across different package.json export conditions. |
| // Test case: regular function BEFORE step function in same declaration | ||
| // This verifies that processing doesn't skip the step function | ||
| const regularArrow = ()=>1, stepAfterRegular = globalThis[Symbol.for("WORKFLOW_USE_STEP")]("step//input.js//stepAfterRegular"); | ||
| const regularArrow = ()=>1, stepAfterRegular = globalThis[Symbol.for("WORKFLOW_USE_STEP")]("step//./input//stepAfterRegular"); |
There was a problem hiding this comment.
Unused variable regularArrow.
| // Test case: regular function BEFORE step function in same declaration | ||
| // This verifies that processing doesn't skip the step function | ||
| const regularArrow = ()=>1, stepAfterRegular = globalThis[Symbol.for("WORKFLOW_USE_STEP")]("step//input.js//stepAfterRegular"); | ||
| const regularArrow = ()=>1, stepAfterRegular = globalThis[Symbol.for("WORKFLOW_USE_STEP")]("step//./input//stepAfterRegular"); |
There was a problem hiding this comment.
Unused variable stepAfterRegular.
| const regularFn = function() { | ||
| return 2; | ||
| }, stepAfterRegularFn = globalThis[Symbol.for("WORKFLOW_USE_STEP")]("step//input.js//stepAfterRegularFn"); | ||
| }, stepAfterRegularFn = globalThis[Symbol.for("WORKFLOW_USE_STEP")]("step//./input//stepAfterRegularFn"); |
There was a problem hiding this comment.
Unused variable stepAfterRegularFn.
| export async function wflow() { | ||
| let count = 42; | ||
| var namedStepWithClosureVars = globalThis[Symbol.for("WORKFLOW_USE_STEP")]("step//input.js//wflow/namedStepWithClosureVars", ()=>({ | ||
| var namedStepWithClosureVars = globalThis[Symbol.for("WORKFLOW_USE_STEP")]("step//./input//wflow/namedStepWithClosureVars", ()=>({ |
There was a problem hiding this comment.
Unused variable namedStepWithClosureVars.
| var namedStep = globalThis[Symbol.for("WORKFLOW_USE_STEP")]("step//input.js//namedStep"); | ||
| export var exportedNamedStep = globalThis[Symbol.for("WORKFLOW_USE_STEP")]("step//input.js//exportedNamedStep"); | ||
| /**__internal_workflows{"steps":{"input.js":{"exportedNamedStep":{"stepId":"step//./input//exportedNamedStep"},"namedStep":{"stepId":"step//./input//namedStep"}}}}*/; | ||
| var namedStep = globalThis[Symbol.for("WORKFLOW_USE_STEP")]("step//./input//namedStep"); |
There was a problem hiding this comment.
Unused variable namedStep.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR changes how the SWC compiler generates IDs for workflows, steps, and classes. Instead of using raw file paths, IDs are now based on Node.js module specifiers when the file belongs to a package (either in
node_modulesor a workspace package).Motivation
Previously, IDs were generated using file paths like
step//src/jobs/order.ts//fetchData. This caused several issues:"workflow"vs"default"conditions inpackage.json), the same import specifier can resolve to different files. Using file paths meant IDs could differ based on which export condition was used.Changes
New ID Format
IDs now use the format
{type}//{modulePath}//{identifier}wheremodulePathis either:point@0.0.1or@myorg/shared@1.2.3for package files./like./src/jobs/orderfor local app filesExamples:
step//workflow@4.0.1-beta.50//fetch(SDK step)step//./workflows/order//processOrder(local step)class//point@0.0.1//Point(package class)class//./src/models/User//User(local class)New Module Specifier Resolution
Added
packages/builders/src/module-specifier.tswhich:node_modulesor a workspace packagepackage.jsonand extracts name/versionSWC Plugin Changes
moduleSpecifieroption to plugin confignaming.rsto support both module specifiers and relative pathsget_module_path()helper that uses specifier when available, falls back to./filenameformatSpecial Cases
__builtin_*): Continue to use just the function name as the ID for stable, version-independent lookup from the workflow VM runtime.Testing
Breaking Changes
This is technically a breaking change for any persisted workflow runs that reference the old ID format. However, since IDs are internal implementation details and not user-facing, this should not affect end users.
Files Changed
packages/builders/src/module-specifier.ts- NEW: Module specifier resolution logicpackages/builders/src/apply-swc-transform.ts- Pass module specifier to SWC pluginpackages/builders/src/base-builder.ts- UsegetImportPathfor virtual entry importspackages/swc-plugin-workflow/transform/src/lib.rs- Accept and use module specifierpackages/swc-plugin-workflow/transform/src/naming.rs- New ID formatting with module pathspackages/swc-plugin-workflow/spec.md- Updated documentationpackages/core/e2e/e2e.test.ts- Updated test assertions for new ID format