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

Replace mapValue with simply undefined #339

Closed
joeduffy opened this issue Sep 14, 2017 · 4 comments
Closed

Replace mapValue with simply undefined #339

joeduffy opened this issue Sep 14, 2017 · 4 comments
Assignees
Labels
impact/usability Something that impacts users' ability to use the product easily and intuitively kind/enhancement Improvements or new features
Milestone

Comments

@joeduffy
Copy link
Member

In #331, we discussed hiding "blocking" code behind a C++ boundary so that we can implement closure serialization in a sane way. As part of that, we began discussing the future of mapValue, since the "promises" nature of how resource properties work will be hidden from JavaScript.

@lukehoban had the suggestion to simply model unavailable values during planning as undefined. Frankly, mapValue just encodes this same thing in a strange and awkward way. It is possible to have conditional code in either model. And arguably the undefined approach feels more like how it ought to be done in idiomatic JavaScript.

For example, initializing some resource property dependent upon another would go like this:

let res1 = new Resource(...);
let res2 = new Resource(..., {
    propA: res1.propA && res1.propA + "x",
});

This work item tracks removing mapValue and simply exposing all resource properties as T | undefined, rather than Computed<T>.

@joeduffy joeduffy added area/core impact/usability Something that impacts users' ability to use the product easily and intuitively kind/enhancement Improvements or new features labels Sep 14, 2017
@joeduffy joeduffy added this to the 0.6 milestone Sep 14, 2017
@joeduffy joeduffy self-assigned this Sep 14, 2017
@joeduffy joeduffy modified the milestones: 0.6, 0.7 Sep 15, 2017
@CyrusNajmabadi
Copy link
Contributor

The one 'nice' thing about the Computed type was that is clearly called out the locatoins where a value could be unavailable during planning, but available during deployment. With "T | undefined" it's less clear that this is trying to represent that dual nature, and makes it seem more like 'undefined' might be something you might always get at any time.

Perhaps we could have the type be "T | undefined" but use a type alias to help make the code better self documented. for example, something like:

type UnavailableAtPlanTime<T> = T | undefined;

Now code may say something like public tableName: UnavailableAtPlanTime<string>

Thoughts?

@joeduffy
Copy link
Member Author

This was definitely one of my major concerns about this approach. I'm not sure which commit you picked up, however we did end up with

type Computed<T> = Promise<T | undefined>;

which is kinda sort close to what you mention, Cyrus. We could go a step further and do

type Maybe<T> = T | undefined;
type Computed<T> = Promise<Maybe<T>>;

Do you think this would help clarify things at least when looking at docs?

@joeduffy
Copy link
Member Author

@CyrusNajmabadi I'm closing this out, but do you think my most recent comment makes sense?

@CyrusNajmabadi
Copy link
Contributor

Yes. I far prefer the new way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/usability Something that impacts users' ability to use the product easily and intuitively kind/enhancement Improvements or new features
Projects
None yet
Development

No branches or pull requests

2 participants