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

Regression on null v1.9.3 to v1.10.0 #265

Closed
deenairn opened this Issue Mar 28, 2017 · 17 comments

Comments

Projects
None yet
4 participants
@deenairn

deenairn commented Mar 28, 2017

I appears there has been a behavioural change between v1.9.3 and v.1.10.0 for the following code:

console.log(numbro(null).format("0.00%"));

1.9.3 fiddle

https://jsfiddle.net/deenairn/L6bygxdw/3/

This outputs: NaN%

1.10.0 fiddle

https://jsfiddle.net/deenairn/1nywjby3/2/

This throws:
Uncaught Error: Invalid input numbro.min.js:117

@BenjaminVanRyseghem

This comment has been minimized.

Owner

BenjaminVanRyseghem commented Mar 28, 2017

yep, this is intended.

Is it a problem?

@BenjaminVanRyseghem

This comment has been minimized.

Owner

BenjaminVanRyseghem commented Mar 28, 2017

I didn't think it was worth a version 2.0.0, but maybe I was wrong

@deenairn

This comment has been minimized.

deenairn commented Mar 28, 2017

We'll just need to adapt to the new behaviour - it was picked up by some unit tests we had written relating to our formatting service that uses numbro under the hood.

What was the a reason why it has changed? It seems sensible to me to return NaN for null rather than throw an Error, it's not a number.

@BenjaminVanRyseghem

This comment has been minimized.

Owner

BenjaminVanRyseghem commented Mar 28, 2017

It was changed because formatting something that was not representing a number felt wrong to me

Sorry for the inconvenience

@deenairn

This comment has been minimized.

deenairn commented Mar 28, 2017

I can see the change was made in commit 02ce326, lines 722 to 725.

Although I can see your point that null is not a number, undefined is also being treated as a number, namely 0, and it is not a number when calling Number(undefined) (it is NaN), so the approach doesn't seem entirely consistent.

As I'm not a maintainer or contributor I'm happy enough to workaround this, although I'm not 100% sold on the principle.

@BenjaminVanRyseghem

This comment has been minimized.

Owner

BenjaminVanRyseghem commented Mar 28, 2017

so the approach doesn't seem entirely consistent.

Not sure to understand what you mean here

As I'm not a maintainer or contributor

Doesn't mean your opinion doesn't matter 😄

@deenairn

This comment has been minimized.

deenairn commented Mar 28, 2017

Lol that is true, I can always submit a PR. All I meant is that undefined, like null, is not a number, and it does not throw an error either. That is what appears inconsistent to me.

@BenjaminVanRyseghem

This comment has been minimized.

Owner

BenjaminVanRyseghem commented Mar 28, 2017

undefined should throw an error now that we don't use Number anymore but rely on typeof. Or am I missing something?

@BenjaminVanRyseghem

This comment has been minimized.

Owner

BenjaminVanRyseghem commented Mar 28, 2017

Lol that is true, I can always submit a PR

You can also just share your point of view 😄

@karczk

This comment has been minimized.

karczk commented Mar 28, 2017

I agree with @deenairn, this is a regression. This code should not throw exception. "NaN", "", "Invalid Number" etc. are much better solutions. Numbro is just a simple library to format/unformat numbers, should not break code execution. Missing breaking change information in release note is another thing.

@deenairn

This comment has been minimized.

deenairn commented Mar 28, 2017

That's not quite true, if you check line 720 of the above commit on numbro.js, you can see the following:

} else if (input === 0 || typeof input === 'undefined') {
              input = 0;
}

This is confirmed in our regression tests - so undefined will return 0.

@karczk

This comment has been minimized.

karczk commented Mar 28, 2017

This behavior can be compared to the moment.js library, that does not throw exception in similar situation:

moment(null).format("L") -> "Invalid date"
moment(undefined).format("L") -> "03/28/2017"
moment().format("L") -> "03/28/2017"

Another argument (no exception):

Number(null) -> 0
Number(undefined) -> NaN

Personally, I would not like to have such a thing (exception on null) even in 2.0.0. I started with numeral, and recently migrated to numbro 1.9.3 without any regressions. Please, consider this breaking change again.

@BenjaminVanRyseghem

This comment has been minimized.

Owner

BenjaminVanRyseghem commented Mar 28, 2017

I can remove it, it's fine for me 😄

@karczk

This comment has been minimized.

karczk commented Mar 28, 2017

Proposition (known from react.js):

  • numbro.js: console.error() or console.warn() on null/undefined (console.error() has full call stack in console), of course with if (console) for IE9,
  • numbro.min.js: no console.error() or console.warn().
@BenjaminVanRyseghem

This comment has been minimized.

Owner

BenjaminVanRyseghem commented Mar 28, 2017

I reverted it for now (and removed the undefined check).

We can discuss what we want later on

gwynjudd added a commit to gwynjudd/numbro that referenced this issue Mar 28, 2017

@ArmorDarks

This comment has been minimized.

ArmorDarks commented Mar 29, 2017

I think it may worth to make it configurable, like numbro.config({ throwOnNan: true }) for cases, when developer want's to be strict and ensure that numbro will never receive NaN.

gwynjudd added a commit to gwynjudd/numbro that referenced this issue Mar 29, 2017

gwynjudd added a commit to gwynjudd/numbro that referenced this issue Mar 29, 2017

@BenjaminVanRyseghem

This comment has been minimized.

Owner

BenjaminVanRyseghem commented Mar 30, 2017

@ArmorDarks that's a good idea 😄

Can you create a new issue to introduce that so it doesn't get lost?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment