-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Don't use instanceof
for RTTI
#1209
Conversation
This change moves us away from using JavaScript RTTI, by way of `instanceof`, for built-in Pulumi types. If we use `instanceof`, then the same logical type loaded from separate copies of the SDK package -- as will happen in SxS scenarios -- are considered different. This isn't actually what we want. The solution is simple: implement our own quasi-RTTI solution, using __pulumi* properties and manual as* and is* functions. Note that we could have skipped the as* and is* functions, but I found that they led to slightly easier to read code. There is one strange thing in here, which I spoke to @CyrusNajmabadi about: SerializedOutput<T>, because it implements Output<T> as an _interface_, did not previously masquerade as an actual Output<T>. In other words, `instanceof` would have returned false, and indeed a few important properties (like promise) are missing. This change preserves that behavior, although I'll admit that this is slightly odd. I suspect we'll want to revisit this as part of #1074. Fixes #1203.
@swgillespie I didn't add your linter, but we should definitely do that as a follow up! |
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.
This seems fine, but it's a thicker interface than I'd normally expect in JS/TS. Especially using a function like asX
to do a cast (but not actually "do" anything") feels unusually heavy for TS code.
Especially since these end up on the public surface area of common things like CustomResource
, I do wonder whether it's not better to leave off these helpers and just rely on directly referencing the property?
Or perhaps just leaving off asX
, since that is purely sugar ( and arguably, it's clearer at callsites what you are doing when you just do this directly, as the "undefined" returned by asX
doesn't get introduced, and the conditional can remain on whether the thing has the type marker or not)?
In particular, if we can use obj is Archive
style user defined type guards, I expect the need for these asX
functions will be even less significant.
sdk/nodejs/asset/archive.ts
Outdated
* Returns true if the given object is an instance of an Archive. This is designed to work even when | ||
* multiple copies of the Pulumi SDK have been loaded into the same process. | ||
*/ | ||
public static isInstance(obj: any): boolean { |
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.
Instead of returning boolean
- return obj is Archive
as a type. This should allow TypeScript to use it as a type guard producing the same typing implications as instanceof
provided previously.
Hopefully that also further reduces the need for asX
functions.
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.
Nice! Will do.
Ah very nice, I actually didn't even know about the |
Right - I think if we can use |
This change adopts `x is T` style of RTTI inquiry, which fits much more nicely with TypeScript's typechecking flow. Thanks to @lukehoban for teaching me a new trick today! :-)
Alright, much nicer! Updated the PR. Thanks for the suggestion. |
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.
LGTM!
* This is internal instead of being truly private, to support mixins and our serialization model. | ||
*/ | ||
// tslint:disable-next-line:variable-name | ||
/* @internal */ public readonly __pulumiOutput?: boolean = true; |
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.
why is this one optional?
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.
The comment on SerializedOutput<T>
(in runtime/closure) explains a bit, however this is certainly subbtle.... SerializedOutput<T>
"implements" Output<T>
, as a mixin, which means, if __pulumiOutput
were required, we'd need a dummy = false
there. I originally went that path because it's more explicit, however, since we decided (after I chatted with @CyrusNajmabadi) not to have SerializedOutput<T>
be an Output<T>
, RTTI-wise (long story), it felt better to not require it to be set explicitly (which would have materially changed serialized code).
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.
👍
This change moves us away from using JavaScript RTTI, by way of
instanceof
, for built-in Pulumi types. If we useinstanceof
,then the same logical type loaded from separate copies of the
SDK package -- as will happen in SxS scenarios -- are considered
different. This isn't actually what we want. The solution is
simple: implement our own quasi-RTTI solution, using __pulumi*
properties and manual as* and is* functions. Note that we could
have skipped the as* and is* functions, but I found that they led
to slightly easier to read code.
There is one strange thing in here, which I spoke to
@CyrusNajmabadi about: SerializedOutput, because it implements
Output as an interface, did not previously masquerade as an
actual Output. In other words,
instanceof
would have returnedfalse, and indeed a few important properties (like promise) are
missing. This change preserves that behavior, although I'll admit
that this is slightly odd. I suspect we'll want to revisit this as
part of #1074.
Fixes #1203.