fix(cli-internal): unify custom-resource dependency detection#14928
Draft
sarayev wants to merge 4 commits into
Draft
fix(cli-internal): unify custom-resource dependency detection#14928sarayev wants to merge 4 commits into
sarayev wants to merge 4 commits into
Conversation
…arg-count mismatch
Custom resource generation used two independent dependency detectors that
could disagree: a regex (extractDependencies) decided whether the resource.ts
wrapper passed a backend argument, while the AST transformer decided whether
the construct constructor received a backend parameter. When the regex missed
a dependency the AST caught (e.g. addResourceDependency with a non-literal
category), the wrapper emitted new Construct(stack, id, backend) against a
2-parameter constructor -> TS2554.
Make AmplifyHelperTransformer.transform return { sourceFile, addedBackendParam }
and drive both the construct constructor parameter and the wrapper argument
from that single signal, guaranteeing they always agree. The now-redundant
regex extractDependencies is removed. The Backend type import decision is also
keyed off addedBackendParam.
…pendency-detection
…ncy test Rewrite the dependency-consistency test to mirror the sibling custom.generator test mocking pattern (mock node:fs/promises and JSONUtilities) instead of real temp dirs, which failed under the package's default jest config. The test still asserts that, for a custom resource with a cross-category dependency, the generated construct constructor parameter count equals the resource.ts constructor-call argument count.
…igration
The gen2-migration custom-resource transformer only recognized the
property-access form `AmplifyHelpers.addResourceDependency(...)`. The
bare named-import form used by customer repos (import { addResourceDependency }
from '@aws-amplify/cli-extensibility-helper' then
const dependencies: AmplifyDependentResourcesAttributes = addResourceDependency(this, ...))
was never detected, so the variable was not registered in dependencyVariables
and hasDependencies stayed false. The Fn.ref rewrite never ran, leaving
undeclared dependencies.* references and causing TS2663.
- Detect bare Identifier addResourceDependency(...) calls in addition to the
existing PropertyAccess form, registering the assigned variable and setting
hasDependencies.
- In the : AmplifyDependentResourcesAttributes typed-declaration branch,
register the variable and set hasDependencies before deleting it, so the
backend ctor param/arg are emitted and dependencies.* refs are rewritten to
backend.*.
Adds unit tests for the bare-import pattern and an integration test in the
dependency-consistency suite exercising the real generator path.
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
1 participant
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.
Problem
Custom resource generation used two independent dependency detectors that could disagree: a regex (
extractDependencies) decided whether theresource.tswrapper passed abackendargument, while the AST transformer decided whether the construct constructor received abackendparameter. When the regex missed a dependency the AST caught (e.g.addResourceDependencywith a non-literalcategory), the wrapper emittednew Construct(stack, id, backend)against a 2-parameter constructor — TS2554.Fix
AmplifyHelperTransformer.transformnow returns{ sourceFile, addedBackendParam }. Both the construct constructor parameter and the wrapper argument are driven from that single signal, so they always agree. Theimport type { Backend }decision is keyed off it too. The now-redundant regexextractDependenciesis removed.Test
amplify-helper-transformer.test.tsupdated to the new return shape, with addedaddedBackendParamtrue/false assertions.custom.generator.dependency-consistency.test.tsdrivesCustomResourceGeneratorwith a variable-category dependency (which the old regex missed) and asserts constructor param count equals wrapper call arg count.Note
Stacked on #14927 (shares
custom.generator.ts); review/merge that first, then rebase this ontodev.Risk
Medium. Unifies on the AST detector (authoritative — it rewrites the references). Guarantees arg/param agreement; the residual TS2663 'undefined dependencies' case depends on customer source and is out of scope here.