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.

Sign up for GitHub

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

Jump to bottom

refactor: Remove unnecessary async methods in crypto package #1949

Closed
maschad opened this issue Aug 9, 2023 · 2 comments · Fixed by #1963
Closed

refactor: Remove unnecessary async methods in crypto package #1949

maschad opened this issue Aug 9, 2023 · 2 comments · Fixed by #1963
Labels
good first issue Good issue for new contributors help wanted Seeking public contribution on this issue P1 High: Likely tackled by core team if no one steps up

Comments

@maschad
Copy link
Member

maschad commented Aug 9, 2023

I think we now have a load of async functions that don't do any async work - we should remove the async keyword from them to prevent promises being created unnecessarily but that can be done in a follow-up.

Originally posted by @achingbrain in #1939 (review)
@maschad maschad added this to js-libp2p Aug 9, 2023
@maschad maschad added help wanted Seeking public contribution on this issue good first issue Good issue for new contributors labels Aug 9, 2023
@maschad maschad moved this to 🛠️ Todo in js-libp2p Aug 9, 2023
@maschad maschad added the P1 High: Likely tackled by core team if no one steps up label Aug 9, 2023
@tabcat
Copy link
Contributor

tabcat commented Aug 10, 2023

Seems like any async functions with // eslint-disable-line require-await are candidates to be changed?

@maschad
Copy link
Member Author

maschad commented Aug 10, 2023 •
edited
Loading

That's a good place to start but not all async functions with that comment are necessarily sync such as the aes-gcm encrypt function which incorrectly has this comment. Would you mind opening a PR to address this?

This was referenced Aug 11, 2023
Merged
Merged
achingbrain pushed a commit that referenced this issue Aug 14, 2023
@tabcat
chore: remove require-await comments in libp2p/crypto (#1958)
78b008c 
These eslint-disable-line comments are no longer needed.
They were added ~3 years ago and do not affect current linting rules.

provenance: 7273739

related: #1949
@p-shahi p-shahi linked a pull request Aug 15, 2023 that will close this issue
refactor!: remove unnecessary async from crypto methods #1963
Merged
@p-shahi p-shahi removed this from js-libp2p Aug 15, 2023
achingbrain added a commit that referenced this issue Nov 27, 2023
@tabcat @maschad @achingbrain
refactor!: remove unnecessary async from crypto methods (#1963)
e2267d4 
Removed async keyword from function which don't:

1) do any async work
2) return a promise
3) have an async browser equivalent

Closes: #1949

---------

Co-authored-by: Chad Nehemiah <chad.nehemiah94@gmail.com>
Co-authored-by: achingbrain <alex@achingbrain.net>
maschad added a commit to maschad/js-libp2p that referenced this issue Nov 27, 2023
@tabcat @maschad @achingbrain
refactor!: remove unnecessary async from crypto methods (libp2p#1963)
0817649 
Removed async keyword from function which don't:

1) do any async work
2) return a promise
3) have an async browser equivalent

Closes: libp2p#1949

---------

Co-authored-by: Chad Nehemiah <chad.nehemiah94@gmail.com>
Co-authored-by: achingbrain <alex@achingbrain.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good issue for new contributors help wanted Seeking public contribution on this issue P1 High: Likely tackled by core team if no one steps up
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor!: remove unnecessary async from crypto methods tabcat/js-libp2p
2 participants
@maschad @tabcat