feat(runtime-html): deactivate while activating

[Sayan751]

Pull Request

📖 Description

This should supersede the PR #1729.

🎫 Issues

👩‍💻 Reviewer Notes

• Take a look at the tests for the use-cases.
• The fix seems to work for the router-lite error recovery in my branch.

📑 Test Plan

⏭ Next Steps

[bigopon]

Nice one @Sayan751. This covers case where we are guaranteed to exit during a phase. Will it work when there' no such guarantee? Example is when there's a long running promise returned in attaching and deactivate is call while that promise is still running? If it doesn't do that, we probably cant supersede #1729.

[Sayan751]

Example is when there's a long running promise returned in attaching and deactivate is call while that promise is still running?

Good point @bigopon! I will add the tests. Thanks for the review.

[jwx]

Nice one @Sayan751. This covers case where we are guaranteed to exit during a phase. Will it work when there' no such guarantee? Example is when there's a long running promise returned in attaching and deactivate is call while that promise is still running? If it doesn't do that, we probably cant supersede #1729.

Yeah, I don't think this covers all use cases that #1729 covers. I'd be surprised if there's a so much easier way to solve those that it's worth searching for a different enough take than that in #1729. 🤷

[bigopon]

Example is when there's a long running promise returned in attaching and deactivate is call while that promise is still running?

Good point @bigopon! I will add the tests. Thanks for the review.

No worries, thanks for the PR. Simplest case I can think of is attaching returns a promise that resolves in 100ms. Then a deactivate is called at 20ms. After that, there shouldn't be any attached hook invoked.

[bigopon]

Nice work @Sayan751 A few more details to iron out for the implementation then I'll start reviewing the test 👍

[codecov]

Codecov Report

Merging #1760 (b0c66dc) into jwx-cancel-activation (06c432a) will increase coverage by 0.17%.
The diff coverage is 94.00%.

@@                    Coverage Diff                    @@
##           jwx-cancel-activation    #1760      +/-   ##
=========================================================
+ Coverage                  87.84%   88.01%   +0.17%     
=========================================================
  Files                        242      242              
  Lines                      22592    22555      -37     
  Branches                    5177     5163      -14     
=========================================================
+ Hits                       19845    19852       +7     
+ Misses                      2747     2703      -44     

Impacted Files	Coverage Δ
packages/runtime-html/src/templating/controller.ts	88.19% <94.00%> (+5.47%)	⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[bigopon]

Nice work on the tests @Sayan751 I have some minor comments for now, will continue soon. Currently I don't agree with the form of the tests, loops are still employed and I anticipate that it's not gonna help us move forward. I'll think of a way to remove the loop and propose that in a separate PR targeting this branch.

[Sayan751]

Currently I don't agree with the form of the tests, loops are still employed and I anticipate that it's not gonna help us move forward. I'll think of a way to remove the loop and propose that in a separate PR targeting this branch.

@bigopon Thanks for the review. I think if we remove the loop, the test suite will explode in size and will soon be unmanageable. I have tried to keep the loop to minimum, not to create much cognitive load.