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

chore(#9582): update datasource bind to not return promise #9583

Merged
merged 2 commits into from
Oct 25, 2024

Conversation

jkuester
Copy link
Contributor

@jkuester jkuester commented Oct 24, 2024

Description

Closes #9582

See issue for details about the change.

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Tested: Unit and/or e2e where appropriate
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

Compose URLs

If Build CI hasn't passed, these may 404:

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

Nice improvement on the API!

One readability suggestion in the implementation.

Comment on lines 109 to 114
return (...p) => new Promise((resolve, reject) => this
.isInitialized()
.then(() => this.dataContext
.bind(fn)(...p)
.then(resolve)
.catch(reject))) as ReturnType<F>;
Copy link
Member

Choose a reason for hiding this comment

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

Personally I find the autoreturn style with multi line body really difficult to follow. Is this just me?

Secondly I think you can remove an indent from lines 113 and 114. My suggestion would be...

Suggested change
return (...p) => new Promise((resolve, reject) => this
.isInitialized()
.then(() => this.dataContext
.bind(fn)(...p)
.then(resolve)
.catch(reject))) as ReturnType<F>;
return (...p) => new Promise((resolve, reject) => {
return this.isInitialized()
.then(() => this.dataContext.bind(fn)(...p))
.then(resolve)
.catch(reject)) as ReturnType<F>;
});

I think I got all the braces right...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😅 Generally I don't mind autoreturns for promise chains (and other places where it is pretty clear where things are going). But, yeah, for this craziness we should make it as readable as possible (even if it means I actually have to type return a few more times.... 😭 ).

Going to take this a couple steps further and explicitly return the constructed Promise as well as assigning the contextual function to a variable to avoid the double-parenthesis from the currying. 👍

Copy link
Contributor

@Benmuiruri Benmuiruri left a comment

Choose a reason for hiding this comment

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

Is that TS code less Janky 😅 ?

I need to do more TS exercism.

But thanks for this, hides the initialization and just gives the function one wants to call.

@jkuester
Copy link
Contributor Author

@Benmuiruri 😆 I should have been more specific! This makes the code consuming cht-datasoure less janky (by encapsulating all the jankyness into the bind function....) 🤦

@jkuester jkuester changed the title chore(#9582): update CHTDatasourceService.bind method to not return promise chore(#9582): update datasource bind to not return promise Oct 25, 2024
@jkuester jkuester merged commit 3bd4759 into master Oct 25, 2024
44 checks passed
@jkuester jkuester deleted the datasource_bind branch October 25, 2024 20:12
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.

Update cht-datasource bind in webapp so it returns a wrapper function instead of a Promise
3 participants