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

Small optimizations + improvements #126

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

juliyvchirkov
Copy link

@juliyvchirkov juliyvchirkov commented Aug 10, 2022

The proposed update

  1. Optimizes functions success and error to get rid of redundant constant declaration
  2. Improves function normalizeOptions, implementing strict type checking for payload object. The method typeof payload === 'object', used before, fails to detect the type clearly, if passed payload inherits Object prototype being de facto non classic Object. This can lead to possible uncaught runime errors and at least results in displaying empty notification. For example, notyf.error(Array.of('They see me rolling')), notyf.error(new String('They see me rolling')) or notyf.success(null), since the check typeof Array.of('They see me rolling'), typeof new String('They see me rolling') and typeof null will identify these Array, String and Null instances as object
  3. Improves function normalizeOptions, correcting the logic of workflow (although it may be subject for discussion) by swapping the order of payload and options during the merge. (options = { ...options, ...payload } => options = { ...payload, ...options }). The point is to assure the notification, produced by options normalization subroutine, is exactly of type passed in 1st argument to normalizeOptions(type, payload) function regardless of properties of payload object. The logic, used before swapping, could result in curious things like invocation of notyf.error({ type : 'success', message : 'They see me rolling' }), which despite the name produces the notification of type success
  4. Improves function open, covering the original code with additional check to assure the argument passed to notyf.open(options) function is of expected classic Object type and this option object contains property message with value of type String. The point is to avoid arrival of empty notifications if for some reason the options hasn't been combined properly. This improvement has an impact only on accidental failures and one still is able to push an empty notification like notyf.error('') or notyf.open({ type : 'success', message : '' }), declaring message as empty string

The proposed update

1) Optimizes functions `success` and `error` to get rid of redundant constant declaration
2.1) Improves function `normalizeOptions`, implementing strict type checking  for `payload` object. The method `typeof payload === 'object'`, used before, fails to detect the type clearly, if passed payload inherits Object prototype being de facto non classic Object. This can lead to possible uncaught runime errors and at least results in displaying empty notification. For example, `notyf.error(Array.of('They see me rolling'))`, `notyf.error(new String('They see me rolling'))` or `notyf.success(null)`, since the check `typeof Array.of('They see me rolling')`, `typeof new String('They see me rolling')` and `typeof null` will identify these Array, String and Null instances as `object`
2.2) Improves function `normalizeOptions`, correcting the logic of workflow (*although it may be subject for discussion*) by swapping the order of `payload` and `options` during the merge.  (`options = { ...options, ...payload }` => `options = { ...payload, ...options }`). The point is to assure the notification, produced by options normalization subroutine, is exactly of type passed in 1st argument to `normalizeOptions(type, payload)` function regardless of properties of payload object. The logic, used before swapping, could result in curious things like invocation of `notyf.error({ type : 'success', message : 'They see me rolling' })`, which despite the name produces the notification of type `success`
3) Improves function `open`, covering the original code with additional check to assure the argument passed to `notyf.open(options)` function is of expected classic Object type and this `option` object contains property `message` with value of type String. The point is to  avoid arrival of empty notifications if for some reason the options hasn't been combined properly. This improvement has an impact only on accidental failures and one still is able to push an empty notification like `notyf.error('')` or `notyf.open({ type : 'success', message : '' })`, declaring message as empty string
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

1 participant