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

fix: add Meteor.applyAsync typings #12645

Merged
merged 2 commits into from May 31, 2023

Conversation

Julusian
Copy link
Contributor

@Julusian Julusian commented May 19, 2023

Typescript typings for Meteor.callAsync were added a while ago, but Meteor.applyAsync was missing.

Meteor.apply has been refactored a bit to allow for sharing types across the two, and jsdoc has been copied from the implementation to make it available to intellisense.

@CLAassistant
Copy link

CLAassistant commented May 19, 2023

CLA assistant check
All committers have signed the CLA.

Comment on lines +230 to +233
asyncCallback?: (
error: global_Error | Meteor.Error | undefined,
result?: Result
) => void
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Grubba27 By looking at this code I think it's possible that asyncApply returns synchronously when the callback is present. It'd have to be investigated separately, but how about not documenting this callback in applyAsync at all? I know I asked about this before (#12196 (comment)), but maybe this should be revisited?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe @denihs is the best person for pinging in this case.
I think applyAsync should not accept a callback.
Regarding the check, that is a great question that we should have discussed.
having the async version with callback can help those projects that just want to migrate and things just work™️ but at the same time blocking the use of it forces the end user to do the migration in the right way I guess

Copy link
Contributor

Choose a reason for hiding this comment

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

We're changing a lot in this function for release 3.0.

This code wouldn't be valid anymore. I'd wait for the final version before creating the types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If theres going to be any further 2.x releases it would be nice to have some types there, I encountered this when updating a project to 2.12 and wanted to replace some Meteor.apply() calls with Meteor.applyAsync().

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then I'd rather add it now, without the callback, and then change it for 3.0.

Copy link
Member

Choose a reason for hiding this comment

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

If I understand the api correctly, the promise is used for the stub result, and the callback is used for the server result.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then this should be documented. But honestly I’d expect it to be the opposite? Mostly because the returned Promise will be used more than the callback, I think.

Copy link
Member

Choose a reason for hiding this comment

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

This is off topic, but for zodern:relay I am considering doing something like this so we don't need callbacks:

const promise = createProject();
const stubResult = await promise.stubPromise;
const serverResult = await promise;

// Another option.
// The returned object (which also is a promise) is the same thing as the previous example,
// but it also has a property for `serverPromise`
const { stubPromise, serverPromise }  = createProject();
const stubResult = await stubPromise;
const serverResult = await serverPromise;

Copy link
Contributor

Choose a reason for hiding this comment

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

The first one is the default for many people.
like this:

const result = await createProject()

Exposing the second one may lead to unnecessary use of the stubPromise?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I missed your message @Grubba27.

The second example still directly returns a promise to get the server result so you still can use const serverResult = await createProject(). If the stub result isn't needed, I assume it generally wouldn't be unnecessarily used since it takes more code. The stubPromise and serverPromise are properties added to the returned promise.

@Grubba27 Grubba27 changed the base branch from devel to release-2.13 May 30, 2023 20:51
@Grubba27 Grubba27 merged commit 31299d6 into meteor:release-2.13 May 31, 2023
5 of 6 checks passed
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

7 participants