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

Fix accidental reference to global "value" variable #3595

Merged
merged 1 commit into from Nov 12, 2016

Conversation

Projects
None yet
5 participants
@davidxmoody
Contributor

davidxmoody commented Nov 11, 2016

Hi,

We just encountered a bug today where moment was incorrectly parsing year and month fields. The bug was introduced while upgrading to moment v2.16.0.

We eventually tracked it down to another library (modular-scale) which was setting a global variable called value to a number. The isNumber function in moment is then mistakenly referencing this global value instead of the input value it gets passed. This pull request fixes the problem.

All the best,
David

@jsf-clabot

This comment has been minimized.

jsf-clabot commented Nov 11, 2016

CLA assistant check
All committers have signed the CLA.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Nov 12, 2016

We have tests for this, can you add the one that would have failed.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Nov 12, 2016

Aaah, its always safe to say typeof bullshit, because that is the proper way to check if something is defined :)

@ichernev

This comment has been minimized.

Contributor

ichernev commented Nov 12, 2016

Merged in 93d1a67

@ichernev ichernev merged commit e172cce into moment:develop Nov 12, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

ichernev added a commit that referenced this pull request Nov 12, 2016

Merge pull request #3595 from davidxmoody:develop
[bugfix] Fix accidental reference to global "value" variable
@captbaritone

This comment has been minimized.

Contributor

captbaritone commented Nov 12, 2016

Sorry about that! I'll do some looking into why jshint didn't catch this for us.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Nov 12, 2016

I know why it didnt caught it. Because its a check to see if its defined :) which it isn't. I dont get how come the tests pass and it still fails under some scenario.

@captbaritone

This comment has been minimized.

Contributor

captbaritone commented Nov 12, 2016

@ichernev It only failed if value was defined as a number in the global scope by another library, which naturally our tests didn't cover.

Some other code in the global scope:

var value = 1;

Our code:

typeof value === 'number' ... // always true!

The tests still passed because the typeof check was just a quick check to cover the 90% case. The toString check covers all the cases we care about, but I believe it's more expensive. Since typeof value === 'number' is always false when value is undefined, our tests were just defaulting to the toString check every time.

@mj1856 mj1856 added this to the 2.17.0 milestone Jan 17, 2017

@mj1856 mj1856 added Bug-fix and removed Pending Next Release labels Jan 17, 2017

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