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

Feature/expire #5

Closed
wants to merge 7 commits into from
Closed

Feature/expire #5

wants to merge 7 commits into from

Conversation

hakod
Copy link

@hakod hakod commented Apr 28, 2019

Hi @dr-dimitru,

I added an expiration feature for while code is still active. Wasn't sure whether to add timestamp for expiration date since the other PR already had that. Please let me know if I should change it to use a timestamp as well. Thanks again.

@dr-dimitru
Copy link
Member

@hakod thank you for the quick PR.

  1. How is it going to work after page reload?
  2. Yes, don't mind another PRs
  3. Please let's discuss an idea before implementing it

@hakod
Copy link
Author

hakod commented Apr 28, 2019

@dr-dimitru No problem! Yeah, I was concerned that the setTimeout would be interrupted. Would a timestamp be the best route and should there be a setInterval to check for expiration every so often? Thanks for reviewing.

@dr-dimitru
Copy link
Member

@hakod I believe we should utilize cookie's expiration in case if cookies are used, use records with .___expireAt prefix holding expirations timestamp, having loop via setInterval(()=>{}, 256) for LocalStorage, and use setTimeout for JS implementation when LocalStorage nor Cookies are available. Does it sounds clear?

@hakod
Copy link
Author

hakod commented Apr 28, 2019

@dr-dimitru Yes, I think I understand it.

@hakod
Copy link
Author

hakod commented Apr 29, 2019

@dr-dimitru Added the cookie expiration using max-age and the prefix to store expiration times. Let me know if there are any changes needed. Thanks.

client-storage.js Outdated Show resolved Hide resolved
client-storage.js Show resolved Hide resolved
client-storage.js Outdated Show resolved Hide resolved
client-storage.js Outdated Show resolved Hide resolved
@dr-dimitru
Copy link
Member

@hakod thank you, added notes

@hakod
Copy link
Author

hakod commented Apr 30, 2019

@dr-dimitru Thanks for the feedback, I really appreciate it. The last note was a bit difficult for me to fix. Should setInterval be triggered whenever page loads? How could I get that working?

Copy link
Member

@dr-dimitru dr-dimitru left a comment

Choose a reason for hiding this comment

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

Check inside constructor if localStorage is supported and setInterval

client-storage.js Outdated Show resolved Hide resolved
dr-dimitru added a commit that referenced this pull request May 22, 2019
- 👷‍♂️ Overall codebase refactoring
- 📦 Reduced size of the package, for minimal footprint and top
performance
- 👨‍💻 Add support for TTL, closing #4 #5 and #2 , thanks to @bobagold
@hakod and @JspHansen
- 👨‍🔬 Tests for new TTL feature
- 🤝 Compatibility with `meteor@1.8.1`
@dr-dimitru
Copy link
Member

Dear @hakod ,

Thank you for your PR and attempt, final solution released as v3.0.0.
Do not hesitate to share your opinion 👨‍💻

@dr-dimitru dr-dimitru closed this May 22, 2019
@hakod
Copy link
Author

hakod commented Jun 28, 2019

Hi @dr-dimitru,
It is interesting you used a while statement instead of setInterval to remove localstorage. Wondering what the reason for that change was.

@dr-dimitru
Copy link
Member

@hakod not using while nor setInterval. Timers are unreliable in Browser's runtime environment. Timers will also pollute JS runtime. So, I'm just checking for expiration date here and remove it from storage if expired, it's also happening when ClientStorage instance is initiated

@hakod
Copy link
Author

hakod commented Jun 28, 2019

OK I think I understand it now. Would it delete without a page refresh too?

@dr-dimitru
Copy link
Member

No, only upon request if expired or on page init. Very similar to how cookies and expiration works in most of browsers

@hakod
Copy link
Author

hakod commented Jun 29, 2019

That makes a lot more sense. Thanks.

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.

None yet

2 participants