-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(ec2): fix error when using Tokens in Vpc.fromLookup() #3740
Conversation
`fromLookup()` will not support lazy values. Throw a clear error message when that is done, and update the documentation to make it clear what should be done instead. Fixes #3600.
Pull Request Checklist
|
*/ | ||
public static fromLookup(scope: Construct, id: string, options: VpcLookupOptions): IVpc { | ||
if (Token.isUnresolved(options.vpcId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Geez. It looks we may be missing this check in tons of places throughout our constructs.
For v2, I wonder if we should strongly type all properties to be string | IResolvableToken
(hypothetical type - may or may not exist yet) to ensure that constructs deal with both cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we are. For many places it doesn't matter though (makes it harder!).
This is again an ancient discussion :).
We explicitly opted to do it this way (encode "magic" values into primitive types) in order to guarantee maximum compatibility with all programming languages. Sum types are not supported in many languages, and emulating them would become unwieldy very quickly (especially if you have to have additional syntax for LITERALLY every property you pass).
Thank you for contributing! Your pull request is now being automatically merged. |
Thank you for contributing! Your pull request is now being automatically merged. |
fromLookup()
will not support lazy values. Throw a clearerror message when that is done, and update the documentation
to make it clear what should be done instead.
Fixes #3600.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license