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(idle,timeout): countdown idle and timeout based on time #131

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

mychalhackman
Copy link
Contributor

@mychalhackman mychalhackman commented Sep 10, 2019

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[ x ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
Timeout and idle do not compensate for time-shifts (e.g.; clock change, device standby).

#110
#112

What is the new behavior?
Idle and timeout are checked every second (instead of once after idle expiry) and compare the system time against the timeout time.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[ x ] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling db87c4c on qhrtech:fix-timeout-idle-time-shift into db0148b on HackedByChinese:master.

@kvetis
Copy link

kvetis commented Aug 26, 2022

This fix is very useful and we've used a "fork" with the almost exact code for years. It would be very helpful to merge this, since we could then upgrade to newer versions. It would also fix #157.

@chinthu
Copy link

chinthu commented Oct 17, 2022

It would be very helpful if you merge this fix.

@mcrtricolor
Copy link

I would also need this fix, thanks.

@gshirley-boku
Copy link

I need this fix too, can we have this fix merged anytime soon? 🙇‍♀️

@StefaniToto
Copy link

Need this fix as well.

@kemmis
Copy link

kemmis commented Sep 12, 2023

Please merge. We need this fix too.

@ZoSo1974
Copy link

I need it too! Please merge ASAP! :)

@Eldorados
Copy link

What is the issue with this PR? 4 years waiting for a much needed fix

Who is able to give the final review and merge?

Copy link

@Eldorados Eldorados left a comment

Choose a reason for hiding this comment

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

Tested on my side and working well

@reunefe
Copy link

reunefe commented Oct 25, 2023

@moribvndvs would it be possible to get this one merged? Or does anybody else has write access to could approve this?

Copy link
Owner

@grbsk grbsk left a comment

Choose a reason for hiding this comment

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

The original model (from AngularJS days) avoided what is here (essentially polling) because of digest performance issues. I had proposed an alternate solution, which was to use the provided WindowInterruptSource along with the default or whatever interrupt sources you're using, which fires an interrupt (thus causing expiry checking) when the window is focused (you could also do focus for window or try visiblitychange). I did not add these to the default sources because it was a bit quirky and was new functionality anyways, and I would rather have let people decide if they needed it until it was rock solid.

Trying the PR myself, this core change seems to work fine without either of those work arounds, but I don't have something to test it exhaustively with. But if others have been trying it and it works, let's try it.

I think the knock on effect of this is the expiry system probably needs to be re-examined, but is currently necessary to synchronize between tabs of the same app. However, we could potentially replace that with a reworked storage event source, use BroadcastChannel, or window.postMessage.

@grbsk grbsk merged commit fb9a6bd into grbsk:master Oct 25, 2023
@grbsk
Copy link
Owner

grbsk commented Oct 25, 2023

Thanks @mychalhackman and all who tested/reviewed it. Sorry it took a lifetime to merge.

@grbsk
Copy link
Owner

grbsk commented Oct 25, 2023

Do we need a minor release for the current target of Angular 15 (and do a separate release with it for 16), or just jump straight to 16?

@kemmis
Copy link

kemmis commented Oct 25, 2023 via email

@Eldorados
Copy link

I am stuck on 15 for a while longer, so a minor release would be much appreciated on my end!

@reunefe
Copy link

reunefe commented Oct 25, 2023

I would do separate releases, 1 for the current target of Angular 15 and a separate one for Angular 16.
Mainly because Angular 16 has some breaking changes that could impact some things:

  • Node upgrade
  • Typescript upgrade
  • etc

Not sure what the impact would be by using it in, for example my case, angular 14. The current target with angular 15 works fine in an application of Angular 14, but maybe that's just me. Unfortunately I'm still stuck to that version for now

@grbsk
Copy link
Owner

grbsk commented Oct 25, 2023

Since it's a fix only, 13.0.1 is the release with this change. It's out on NPM now.

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.