Skip to content

refactor(heart): bind class methods and make beat async #5142

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

Merged
merged 7 commits into from
May 4, 2022

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Apr 28, 2022

While implementing #5122, I didn't bind the class methods, which meant passing them to another function them calling this in them would result in undefined.

I also cleaned up the hack I used before with some async logic.

References

Partially related to #5131

@github-actions
Copy link

github-actions bot commented Apr 28, 2022

✨ code-server docs for PR #5142 is ready! It will be updated on every commit.

@jsjoeio jsjoeio temporarily deployed to npm April 28, 2022 21:33 Inactive
@codecov
Copy link

codecov bot commented Apr 28, 2022

Codecov Report

Merging #5142 (6a7ef72) into main (7027ec7) will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5142      +/-   ##
==========================================
+ Coverage   71.62%   71.73%   +0.10%     
==========================================
  Files          30       30              
  Lines        1688     1691       +3     
  Branches      375      375              
==========================================
+ Hits         1209     1213       +4     
+ Misses        410      409       -1     
  Partials       69       69              
Impacted Files Coverage Δ
src/node/routes/index.ts 80.80% <ø> (ø)
src/node/heart.ts 100.00% <100.00%> (+3.70%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7027ec7...6a7ef72. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Apr 28, 2022

✨ code-server dev build published to npm for PR #5142!

  • Last publish status: success
  • Commit: 6a7ef72

To install in a local project, run:

npm install @coder/code-server-pr@5142

To install globally, run:

npm install -g @coder/code-server-pr@5142

@jsjoeio jsjoeio temporarily deployed to npm April 28, 2022 22:10 Inactive
@jsjoeio jsjoeio force-pushed the jsjoeio/fix-heart-undefined branch 2 times, most recently from 6627b0b to dd2829a Compare April 28, 2022 22:23
jsjoeio added 4 commits April 28, 2022 15:24
This allows us to properly await heart.beat() in our tests and remove
the HACK I added before.
This allows the functions to maintain access to the Heart instance (or
`this`) even when they are passed to other functions. We do this because
we pass both `isActive` and `beat` to `heartbeatTimer`.
@jsjoeio jsjoeio force-pushed the jsjoeio/fix-heart-undefined branch from dd2829a to 4d29cd5 Compare April 28, 2022 22:26
@jsjoeio jsjoeio self-assigned this Apr 28, 2022
@jsjoeio jsjoeio added the testing Anything related to testing label Apr 28, 2022
@jsjoeio jsjoeio temporarily deployed to npm April 28, 2022 22:31 Inactive
@jsjoeio jsjoeio changed the title wip refactor(heart): bind class methods and make beat async Apr 28, 2022
@jsjoeio jsjoeio marked this pull request as ready for review April 28, 2022 22:32
@jsjoeio jsjoeio requested a review from a team April 28, 2022 22:32
@jsjoeio jsjoeio added this to the April 2022 milestone Apr 28, 2022
@jsjoeio jsjoeio requested a review from code-asher May 4, 2022 22:27
@jsjoeio jsjoeio temporarily deployed to npm May 4, 2022 22:32 Inactive
@jsjoeio jsjoeio temporarily deployed to npm May 4, 2022 22:49 Inactive
@jsjoeio jsjoeio merged commit 88e971c into main May 4, 2022
@jsjoeio jsjoeio deleted the jsjoeio/fix-heart-undefined branch May 4, 2022 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Anything related to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants