refactor(core): Reorganizing internal/exported utils#286
Conversation
06d0df8 to
a48bbca
Compare
bnchrch
left a comment
There was a problem hiding this comment.
Nice follow-up. Splitting the utils grab-bag into themed #helpers/* files (tree-shakeable) and exposing a curated Objects on sdk/helpers is clean, and dropping ./sdk_utils closes the loop from #270. One real design question on the metadata write side, plus a sandbox-boundary note, inline.
| }; | ||
|
|
||
| setMetadata( wrapper, { name, description, inputSchema, outputSchema, type: ComponentType.STEP, options } ); | ||
| assignImmutableProperty( wrapper, METADATA_ACCESS_SYMBOL, { name, description, inputSchema, outputSchema, type: ComponentType.STEP, options } ); |
There was a problem hiding this comment.
❓ Dropping setMetadata means each factory now open-codes assignImmutableProperty( wrapper, METADATA_ACCESS_SYMBOL, ... ) and imports the symbol (here, plus evaluator.js, workflow.js, internal_activities x2). The read side (ComponentMetadata.has/getName) still hides the symbol, so reads and writes are now asymmetric and the symbol leaks to 5 spots. assignImmutableProperty is a nice generic, but WDYT about a thin domain writer over it that pairs with the reader, e.g. ComponentMetadata.set( fn, attrs ) (or setComponentMetadata)? Keeps the symbol and the immutability in one place.
There was a problem hiding this comment.
I have added a new helper function to create the component, which encapsulates better the behavior.
| * @param {array} arr | ||
| * @returns {array} | ||
| */ | ||
| export const shuffleArray = arr => arr |
There was a problem hiding this comment.
🤔 workflow.js imports #helpers/object, so #helpers modules get bundled into the Temporal workflow sandbox, the same sandbox-safe constraint the sdk/helpers README spells out, but nothing says so for #helpers. shuffleArray's Math.random() is harmless today (only worker-side callers), but it's exactly what that guardrail catches. Worth a line that #helpers modules reachable from workflow code must stay sandbox-safe? (Minor aside: "helpers" now names two things, internal #helpers/* and exposed ./sdk/helpers, fine since the prefix disambiguates, just noting it after #270.)
There was a problem hiding this comment.
I honestly think that it goes without saying, because the code doesnt even starts if you import stuff that Temporal considers non-deterministic, hence why I opted for the tree shaking method.
I made it extra clear on the sdk/helpers because those will be read elsewhere.
There was a problem hiding this comment.
One idea is we have an explicit temporal-sandbox directory for any files that are meant to be sandbox safe.
but thats a very minor idea and out of scope a bit. not blocking
17bc183 to
a11ff5b
Compare
Summary
./utilsand./internal_utilsunder./helpers(#helpers/*import). This module is organized in sub files, to allow tree-shaking imports;Objectsnamespace tosdk/helpers, as those are used by other SDK modules. Added re-exports of the internal helpers;./sdk_utilsaltogether;allSettledWithTimeoutfunction, as it was not being used.Test plan