spencermountain / spacetime

A lightweight javascript timezone library
http://spacetime.how/
Other
3.97k stars 183 forks source link

silent option is not working #308

Closed joshball closed 2 years ago

joshball commented 2 years ago

I cannot trigger an Ambiguity Warning by setting the silentoption to false

It looks like there is some faulty logic in constructor:

this.silent = options.silent || true

That is strange logic to parse as written as options.silent is passed in as false according to the README:

let s = spacetime(123456, 'UTC', {
  silent: false
})

I guess a few possible fixes:

  1. Assume a warning by default, and make 'silent' something you have to pass in to avoid the warning (probably the best, but might piss people off)
  2. Rename the flag to warn, and set it to true to get the warn
  3. Or just fix it in place:

Add a new method:

const isSilentSetToFalse = (opt) => typeof opt.silent === 'boolean' && opt.silent === false;

Then something like this:

this.silent = true 
if(isSilentSetToFalse(options)) {options.silent = false}

Or something equally as confusing:

this.silent = !isSilentSetToFalse(options)

And of course, add a test or two.

spencermountain commented 2 years ago

Hey Josh! Great idea. Pr welcomed!

Mitsunee commented 2 years ago

would a simple change to this solve the issue?

this.silent = options.silent ?? true

That is what I use in modern-diacritics as both current node versions as well as all modern browsers now support the ?? operator (more info: Nullish coalescing operator (??) on MDN)

spencermountain commented 2 years ago

yep! that sounds good to me. thanks