Repository navigation
chore: move packages to packages/ directory, use npm deps instead of file deps - #7
Merged
Merged
Conversation
anthonyting
force-pushed
the
dev-anthony
branch
4 times, most recently
from
September 3, 2025 22:47
e54ce3a to
030b04c
Compare
anthonyting
force-pushed
the
dev-anthony
branch
from
September 3, 2025 22:53
030b04c to
ad22062
Compare
SilanHe
approved these changes
Sep 3, 2025
ParidelPooya
added a commit
that referenced
this pull request
Aug 2, 2026
Review items 7, 8, 9, 10, 13. #7 The update path only ever sent DurableConfig, so Runtime/Handler/Timeout/ MemorySize were set exclusively in the create branch. Redeploying onto a pre-existing function replaced its code with ours while leaving it on whatever runtime and handler it already had — the generated bundle needs this runtime and index.handler, so it failed at invoke for a reason the deploy output never explained. Update now converges the same shape a create produces. Role is deliberately NOT repointed: it may be user-managed, and silently changing it is destructive in a way a code update is not. PutRolePolicy also guessed `${functionName}-role`, which is only correct when we created the role in the same run. On the update path ensureExecutionRole never runs, so the guess could name a nonexistent role (swallowed) or an unrelated one that matched the pattern. It now resolves the function's effective role from GetFunctionConfiguration and derives the name from that ARN (last path segment, since role ARNs may carry a path). #8 Confirmed and it is the worst of these: Succeed and Fail both mapped to a bare end node, and with no endMode codegen emits a return — so EVERY imported Fail reported the execution as SUCCEEDED. Error and Cause were dropped, so the reason disappeared too. Fail now emits endMode "throw" carrying Error and Cause. aslImport.test.ts asserted the old shape and has been corrected. #9 One correction: the join is NOT the bug. Codegen already splits a comma-separated errorType and ORs the instanceof checks, so ["States.Timeout","States.ALL"] produced two separate checks, not one bad string. The real defect is that ASL error NAMES are not JavaScript classes, so `err instanceof States.Timeout` is a ReferenceError at runtime, and States.ALL is a catch-all rather than a type. Now: States.ALL becomes a genuine catch-all, other States.* names are dropped with a note, and non-States names are kept since a custom ASL error name plausibly is the thrown class. #10 Agreed this is the largest ASL gap. InputPath/OutputPath/ResultPath/ ResultSelector were declared and never read, with no note — an import could look right on the canvas and route data completely differently. Translating JSONPath data-flow is not a small change and guessing would be worse, but dropping it SILENTLY is the indefensible part, so every dropped field is now reported, plus TimeoutSeconds/HeartbeatSeconds (which were not even declared), JSONata QueryLanguage, and Retry/Catch on non-Task states. #13 grantInferredPermissions defaulted to true with resources: ["*"]. Split it: statements with resolvable resources (chainInvoke targets) are still granted, and wildcard statements are withheld and reported as a CDK warning naming the exact actions. New grantWildcardPermissions opt-in restores the old behaviour. A construct should not hand out wildcard access off a regex match. 22 new tests. cdk 257, insight-vscode 413, all green.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Issue Link, if available
Description
chore: move packages to
packages/directory, use npm deps instead of file deps.This is the standard approach for npm workspaces:
Also using npm links with
*instead of file links. When publishing, we'll need to use npm links (and use an actual version instead of*)Demo/Screenshots
Checklist
Testing
Unit Tests
Have unit tests been written for these changes? N/A
Integration Tests
Have integration tests been written for these changes? N/A
Examples
Has a new example been added for the change? (if applicable) N/A