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

Suggestions #33

Open
9 tasks
djalilhebal opened this issue Dec 25, 2023 · 0 comments
Open
9 tasks

Suggestions #33

djalilhebal opened this issue Dec 25, 2023 · 0 comments

Comments

@djalilhebal
Copy link

  • Don't call init() from the constructor.
    Let the user call it or another function called start, etc.

  • Why not define defaultTimerInSeconds as a const, and put it next to the default event?

  • Consider renaming defaultTimerInSeconds in the constructor to something that does not include default. Maybe timerInSeconds or intervalInSeconds.

  • Change either timerInSeconds or timerInMs.
    eventHandler(fn: CallbackFunctionVariadic, timerInMs: number) says it accepts the timer in ms, but you are passing it in seconds.
    Why are we passing fn and timer anyways? We can access them as instance properties.

  • Because of how the code is written (mostly because of Point No. 1), it's not possible (in an acceptable way anyway) to specify what events we want to listen to.
    Add it as an arg to the constructor or, even better, use a parameter to specify params.

  • The lib's logic, which is basically a debounce function, feels off.

    1. It won't start the timer until the user generates an event. What if the user opens something like youtube that starts playing the vid automatically?
    2. It will keep triggering, with no stop condition. What if we have already logged out the user and displayed an annoying notification? Print it again and try to invalidate an invalid token/session?

  • In the readme, it suggests including the script v0.0.1, but the latest version is v0.0.2

  • https://unpkg.com/browse/hamid.js@0.0.2/dist/lib.js links to a web page. The correct link to the raw file should be:
    https://unpkg.com/hamid.js@0.0.1/dist/lib.js (notice that we remove "browse").

  • Even this script is not compiled properly to be included on the web.
    To actually make it work (as a hack):

`<script>
var exports = {};
</script>
<script src="https://unpkg.com/hamid.js@0.0.2/dist/lib.js"></script>
``
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

No branches or pull requests

1 participant