Skip to content

Instantly share code, notes, and snippets.

@jbreckmckye
Last active January 11, 2017 15:02
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save jbreckmckye/6c650c486116ffdf7715023499451147 to your computer and use it in GitHub Desktop.
Save jbreckmckye/6c650c486116ffdf7715023499451147 to your computer and use it in GitHub Desktop.
PR

What I am changing

This PR fixes a bug in the settings/motivations page:

image

Presently, it isn't possible to change the motivation more than once per page load. On the second change, we throw an exception:

vendors.min.js:9 Raven: Exception  TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at CoreUtils.compare (https://localhost:8000/js/app/all.min.js?cb=1482319557537:3:31176)
    at https://localhost:8000/js/app/all.min.js?cb=1482319557537:3:31397
    at Array.forEach (native)
    at CoreUtils.compare (https://localhost:8000/js/app/all.min.js?cb=1482319557537:3:31190)
    at https://localhost:8000/js/app/all.min.js?cb=1482319557537:3:31397
    at Array.forEach (native)
    at CoreUtils.compare (https://localhost:8000/js/app/all.min.js?cb=1482319557537:3:31190)
    at User.update (https://localhost:8000/js/app/all.min.js?cb=1482319557537:6:5200)

What's the cause?

When we update a user, we pass an old and new user object to CoreUtils.compare, which gathers us a list of all the changed properties. These will get sent to the API.

Compare doesn't just look at the top-level properties of objects, but the nested ones too. It will recursively inspect these for properties that are nested further still. This works well enough so long as the two compared objects have the same data structure.

Here, compare gets caught when it inspects the user's dateJoined property. This is a moment object with deeply nested, private properties, and these don't always play by compare's rules. This trips the function up.

However, Compare isn't supposed to deeply inspect dates in the first place. There's a check for this (instanceof Date), but moment objects don't inherit from Date, and fail the guard. So I've updated this condition so that Dates and moments are handled in the same way.

Changing date handling

But that's not all. It's great that moments now get compared like Dates, but the way that is actually done didn't please me:

else if (original[key] !== changed[key]) {...}

This code compares dates by object equality - i.e. that they have same pointer in memory. Two instances of Date with the same value will therefore seem different. This meant that whenever the user was updated, we'd tell the API to update various date properties that hadn't actually changed at all:

image

The only one of these I actually changed was my journey_focus.

I have therefore updated the comparison to now work by UNIX epoch. This is a count of milliseconds since January 1st, 1970 UTC, and is a timezone-independent way to compare times.

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