Skip to content
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

Produce a strongly-typed 'unwrap' function to help with deep unwrapping of Input values. #1915

Merged
merged 16 commits into from
Sep 12, 2018

Conversation

CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Sep 10, 2018

Expected usage is:

var transformed = pulumi.unwrap(someVal).apply(unwrapped => {
    // Do whatever you want now.  'unwrapped' will contain no outputs/promises inside
    // here, so you can easily do whatever sort of transformation is most convenient.
});

var someResource = new SomeResource(name, { data: transformed ... });

TypeScript will properly figure out the type of 'unwrapped' based on the type of 'someVal', ensuring that you cannot misuse the properties on the returned value. The returned object will itself be an Output, ensuring that all interior 'Resources' have been appropriately lifted. That way, when 'transformed' is passed on, we will accurately keep track of the dependencies.

@CyrusNajmabadi
Copy link
Contributor Author

This can be reviewed now.

Copy link
Member

@pgavlin pgavlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This is awesome.

@joeduffy
Copy link
Member

Should we be moving these to be static functions on Output? I think we agreed to do that with all just recently, since it's a bit hard to discover as-is based on user feedback?

Copy link
Contributor

@swgillespie swgillespie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome!

@CyrusNajmabadi
Copy link
Contributor Author

Should we be moving these to be static functions on Output? I think we agreed to do that with all just recently, since it's a bit hard to discover as-is based on user feedback?

Good point. I'll make that change. Would you also like me to add .all to Output as well to match Promise.all? Or would you prefer that as a separate PR?

@lukehoban
Copy link
Member

I think we have to be a little careful/intentional about moving these functions. We spent a lot of time on the original design here - and we should make sure that any change leads to a rationale complete set of operations and a net better result.

In particular, today we have pulumi.all and pulumi.output. We need a proposal for how to map these (plus anything new) into an alternate design if we really want to consider that.

@CyrusNajmabadi
Copy link
Contributor Author

Let's chat tomorrow about options.

@CyrusNajmabadi
Copy link
Contributor Author

Luke and i discussed this at length. Our takeaway was to do something different. Namely, 'unwrap' will not be exposed as a function itself. Instead, 'pulumi.output' will just always unwrap values and will return the right type. This should dramatically simplify our programming model around Inputs/Promises, and will benefit people who are already using .output today. Instead of 'output' effectively only converting the very first input level, it will do that conversion deeply.

Note that this should be a safe change to make for existing code. Specifically, the type one gets back is always narrower than teh type you used to get back. So, where previous code may have returned something like Output<{ a: Input<number> }> the new return type will be Output<{ a: number }>. However, this new type is always safe to use anywhere where the old type could have been used.

All that will happen now is that instead of clients having to then wrap the a property in another Output, they can just use a as a number directly.

Copy link
Member

@lukehoban lukehoban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

* // properly maintained.
* var someResource = new SomeResource(name, { data: transformed ... });
* ```
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should simplify the comment here (and fix it up to actually be about output).

I would just say something like "pulumi.output takes any Input value and converts it into an Output, deeply unwrapping nested Input values as necessary". And then include an example.

*/
export function output<T>(val: Input<T>): Output<Unwrap<T>>;
export function output<T>(val: Input<T> | undefined): Output<Unwrap<T | undefined>>;
export function output<T>(val: Input<T | undefined>): Output<Unwrap<T | undefined>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is unchanged - but I'm unclear on why these set of signatures was every required. The second seems to unnecessarily overlap with the first, and the implementation signture seems to unnecessarily include things that aren't valid in either of the exposed signatures. Not a big deal - but if we can clean up - may as well while we are touching this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, something seems wonky here. I'll see if we can clean this up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. So i looked at this, and i think this is waht we want. First, let's completely ignore the impl signatures, it's irrelevant. So we have these two sigs:

output<T>(val: Input<T>): Output<T>;
output<T>(val: Input<T> | undefined): Output<T | undefined>;

(i'm leaving out 'Unwrap' as it doesn't help with the explanation, and it keeps things simpler).

Now, say you only had the first form. But say you had the following type somewhere:

interface ClusterArgs {
    name: Input<string>;
    isPublic?: Input<boolean>;
    // ...
}

const c: ClusterArgs = //...
const val = output(c.isPublic).apply(...);

If you only had the first 'output' form this code would not be legal. TS would complain that you were passing an Input<boolean> | undefined to something that only took an Input<boolean>.

Now, say we only had the second form of output. That would not be a great situation either in the following code:

const c: ClusterArgs = //...
const val = output(c.name).apply(name => /* here name is possibly undefined */);

Now, every time you call .apply, you might have to deal with something 'undefined' even if there was no way that would happen.

The two forms allow us to express this duality cleanly. If you have a non-optional Input property, then you call the first form, and you get an Output which gives you a non-optional inner value. If you have an optional property, you call the second form, and you get a possibly undefined inner value (as expected).

In both cases you get an 'Output' though, allowing you to always feel safe doing output(val).apply(...).

--

Does that make sense @lukehoban ? If you feel otherwise, let me know what you're thinking.

@joeduffy
Copy link
Member

Independent of this, it is still quite bizarre to have these as top level functions. What's the plan?

@CyrusNajmabadi
Copy link
Contributor Author

Independent of this, it is still quite bizarre to have these as top level functions. What's the plan?

I'm very wary of necessarily changing things here. My plan, for now, would be: wait on getting more feedback.

Thes major problems here is that:

  1. no matter what we pick, we still have to educate people.
  2. If we don't get with top level functions, we increase the burden for consumers greatly. For example, people will either have ot say something like pulumi.Output.all(...), or they'll have to have multiple imports instead of being able to just easily do import "@pulumi/pulumi".
  3. I'm not sure it's that bizarre to have top level functions in the JS/TS work. It's super common to pull in a dependent module and then to have all sorts of useful stuff hanging off of it.

Right now, we have no further plan as there isn't a clear indication that this is a problem and there's no great solution that really feels like it would make things better.

Note: a great thing about the current PR is that it actually doesn't increase our surface area. It only narrows and improves the existing surface in (we hope) a totally compatible fashion. So this doesn't make any existing issues (liek top level functions) worse. But it does make existing patterns much better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants