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

Allow users to export a top-level function to serve as the entrypoint to their pulumi app. #3321

Merged
merged 16 commits into from
Dec 9, 2019

Conversation

CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Oct 10, 2019

Optional part of the work to remove deasync and mitigate OSX crashes/hangs seen in the wild. That work is tracked here: #3318

This PR will make life easier if we run into customers that we have to tell to switch to using async/await when using Providers and data-source invokes.

The new usage is like this:

module.exports = async () => {
    // user code.
}

or

export = async () => {
     // user code
}

The benefit here is that this is now a much simpler and easier to use programming model for async programming (up until nodejs/ecmascript/ts get top-level await). We will automatically call and await this function for the user, and they can then easily write code in the function that contains await itself.

The alternative is to force users to write stuff like this:

(async function f() {
   // user code
})();

However, besides being pretty ugly, it's also very difficult to accomplish certain things with this pattern (like exporting a set of outputs). With the new form you can just do:

module.exports = async () => {
    // user code.

   // The returned value is the `stack exports` objects.  For each name/value in the returned object,
   // a stack export will be created.  In this case, two exports `ex1` and `ex2` will be created.
   // The values of these exports can be values that pulumi supports for exports.  Including Promises
   // and Outputs themselves.
   return { ex1: whatever, ex2: whatever };
}

Contrasted with:

const p = (async function f() {
   // user code

   return { ex1: whatever, ex2: whatever };
})();

export ex1 = p.then(v => v.ex1);
export ex2 = p.then(v => v.ex2);

Compared to this, the new form is much cleaner and simpler.

sdk/nodejs/Makefile Outdated Show resolved Hide resolved
@CyrusNajmabadi
Copy link
Contributor Author

CyrusNajmabadi commented Oct 10, 2019

@lukehoban For eyes on this. Note that i've made this an independent PR from the rest of the sync-invokes work I'm doing. I personally just think this would be a nice-to-have. We don't even have to talk about it that much. It's basically only there for the point in time that some customer says "man... i need to do some async stuff, but JS/TS is such a pain for that!". We can now say: "actually, it's not so bad, just export this top level async function and you will be good".

Note: this is not even that painful with node modules as they exist today (as long as you are on node 10 or above) if you are doing this in multiple files.

That's because you can write this:

// mod1.ts
export default async() => {

}

// mod2.
import mod1Func from "./mod1";

export default async () {
    const mod1 = await mod1Func();
};

In this way, you can compose async modules that are all exporting async funcs. It's not great, but it's not terribly either.

Until we get actual top-level async in nodejs (which does look like it's making reasonable progress), this can help users out.

@lukehoban
Copy link
Member

@jen20 curious your thoughts on this (as you’ve expressed frustration around this issue in the past)? This appears to be the best we can currently do at making it easier to do async work at top level. Would this be a reasonable answer - or would this still feel too cumbersome?

One question - where would we document this? If we do it - I do want to make sure it is cleanly documented such that we can point to a primary reference whenever we need to suggest this pattern.

await mod1func

I assume this needs to be await mod1Func()

Also - since the main value here is about stack exports - would be good to include some examples in the PR description that show the pattern of how you expose stack exports using this pattern.

@jen20
Copy link
Contributor

jen20 commented Oct 11, 2019

@lukehoban This looks good to me, although there were reservations expressed about that in #1611 when it came up last?

@CyrusNajmabadi
Copy link
Contributor Author

@lukehoban This looks good to me, although there were reservations expressed about that in #1611 when it came up last?

My primary reservations back then were a lot about if this was even possible to do. Back then, even returning a top level promise was something that was a bit scary. But we ended up handling that without any issues. Also, there was a main question in my mind about how important this was to solve.

With the potential for us to be pushing async more heavily on users in the near future, i do think it would be valuable now to make this even easier.

@CyrusNajmabadi
Copy link
Contributor Author

I assume this needs to be await mod1Func()

Yup! will fix.

Also - since the main value here is about stack exports - would be good to include some examples in the PR description that show the pattern of how you expose stack exports using this pattern.

Yup. Will do!

@CyrusNajmabadi
Copy link
Contributor Author

Also - since the main value here is about stack exports - would be good to include some examples in the PR description that show the pattern of how you expose stack exports using this pattern.

Oh wait, i already ahve those in the description. But i'll try to explain it more clearly!

@CyrusNajmabadi
Copy link
Contributor Author

Thanks for the signoff @pgavlin . I'm going to keep this open till monday as i believe this work will tie into our views on what the right near-term and medium-term solutions are for asynchronous in pulumi.

CHANGELOG.md Outdated Show resolved Hide resolved
@CyrusNajmabadi CyrusNajmabadi merged commit 048acc2 into master Dec 9, 2019
@pulumi-bot pulumi-bot deleted the exportFunctions branch December 9, 2019 19:28
@justinvp
Copy link
Member

justinvp commented Dec 9, 2019

Hmm, looking at the merged commit shows the CHANGELOG entry under 1.6.0.

@CyrusNajmabadi
Copy link
Contributor Author

Darn. Will fix!

@dustinfarris
Copy link

Looks like TypeScript 3.8 will help with this.

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

6 participants