Fix/commontools global reference #1582
Conversation
ce98584 to
7f4152f
Compare
There was a problem hiding this comment.
cubic analysis
2 issues found across 44 files • Review in cubic
React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai to give feedback, ask questions, or re-run the review.
There was a problem hiding this comment.
Test fixture imports commontools directly instead of using the global reference globalThis.__CT_COMMONTOOLS, which does not match the intended transformation pattern described in the PR summary. Update the import to reflect the new global reference usage for accurate test coverage.
Prompt for AI agents
Address the following comment on packages/js-runtime/test/fixtures/schema-transform/recipe-output-schema.expected.ts at line 2:
<comment>Test fixture imports commontools directly instead of using the global reference globalThis.__CT_COMMONTOOLS, which does not match the intended transformation pattern described in the PR summary. Update the import to reflect the new global reference usage for accurate test coverage.</comment>
<file context>
@@ -0,0 +1,53 @@
+/// <cts-enable />
+import { recipe, Cell, UI, NAME, h, derive, JSONSchema } from "commontools";
+// Simple types to test recipe output schema transformation
+interface RecipeInput {
</file context>
| import { recipe, Cell, UI, NAME, h, derive, JSONSchema } from "commontools"; | |
| +const { recipe, Cell, UI, NAME, h, derive, JSONSchema } = globalThis.__CT_COMMONTOOLS; |
recipes/derives.tsx
Outdated
There was a problem hiding this comment.
Array.splice returns the removed elements, not the mutated array, so after deletion items will be set to the array of deleted items instead of the updated list.
Prompt for AI agents
Address the following comment on recipes/derives.tsx at line 195:
<comment>Array.splice returns the removed elements, not the mutated array, so after deletion items will be set to the array of deleted items instead of the updated list.</comment>
<file context>
@@ -0,0 +1,251 @@
+import { Cell, Default, derive, h, handler, NAME, recipe, UI, JSONSchema } from "commontools";
+type TodoItem = {
+ title: Default<string, "">;
+ done: Default<boolean, false>;
+};
+type TodoList = {
+ title: Default<string, "untitled">;
+ items: Default<TodoItem[], []>;
+};
</file context>
53808e6 to
c1943b9
Compare
c1943b9 to
f948dfb
Compare
|
this might not be ready - but I wanted @jsantell feedback on the idea of exposing CT as a global |
When recipes containing transformed OpaqueRef code are exported from one module and imported into another, the AMD module parameter `commontools_1` is not in scope, causing "ReferenceError: commontools_1 is not defined" at runtime. Root cause: AMD module parameters like `commontools_1` only exist within the define callback scope of their module. When a transformed recipe is exported and called from another module, it executes outside this scope. Solution implemented: 1. Bundle wrapper sets `globalThis.__CT_COMMONTOOLS = runtimeDeps.commontools` to make CommonTools functions globally available 2. Transformer now returns `(globalThis.__CT_COMMONTOOLS)` instead of `commontools_1` for all CommonTools function references 3. All transformed code now uses the global reference, ensuring functions are available regardless of module boundaries Changes: - Modified bundle.ts to inject global CommonTools reference - Updated imports.ts getCommonToolsModuleAlias() to return global reference - Added helper functions in opaque-ref.ts for checking import needs - Updated 15 test fixtures to expect (globalThis.__CT_COMMONTOOLS) - Added test case for recipe import scenario - Updated test assertions in opaque-ref.test.ts This fix ensures recipes with transformed code work correctly when imported and called from other modules, resolving the module scope issue. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
f948dfb to
3eb8687
Compare
There was a problem hiding this comment.
What was the issue with the current transformation? This would clobber the global on every bundle execution due to no sandboxing, each "instance" sharing the same (most recent) import object, though not any less "secure" than before. The bundle would still be sandboxable at least (when executed within a sandbox)
| const BUNDLE_PRE = stripNewLines(` | ||
| ((runtimeDeps={}) => { | ||
| const { define, require } = (${getAMDLoader.toString()})(); | ||
| globalThis.__CT_COMMONTOOLS = runtimeDeps.commontools; |
There was a problem hiding this comment.
Add some comments here; previously this package was completely unaware of commontools runtime, add comments that explain the connection
|
closing |
Summary by cubic
Updated the codebase to use a global reference for CommonTools, fixing issues with recipe imports and ensuring all transformed code accesses CommonTools via globalThis.__CT_COMMONTOOLS.