Skip to content

Conversation

@laurenzlong
Copy link
Contributor

Description

Fixes bug where timestamp values that didn't have millisecond or nanosecond precision resulted in "Invalid date" when event.data.data() is called.

Code sample

src/encoder.ts Outdated
nanoString = '000000000';
} else {
nanoString = timeString.substring(20, timeString.length - 1);
nanoString += _.reduce(new Array(9 - nanoString.length), i => {return i + '0';}, '');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woah... String -> array -> reduce -> parseInt? It may be the c programmer in me, but string manipulation of numbers feels really wonky.

why not something like

let nanos: 0;
if (timeString.length > 20) {
  const nanoString = timeString.substring(20, timeString.length - 1);
  const trailingZeros = 9 - nanoString.length;
  nanos = parseInt(nanoString, 10) * Math.pow(10, trailingZeroes);
}

@inlined inlined assigned laurenzlong and unassigned inlined Oct 23, 2017
@laurenzlong
Copy link
Contributor Author

@inlined That is way cleaner, thank you.

@laurenzlong laurenzlong assigned inlined and unassigned laurenzlong Oct 23, 2017
src/encoder.ts Outdated
}
let proto = {

return {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also say return {seconds, nanos}
It felt weird to me at first, but it starts to feel pretty natural and expressive. It also encourages you to use variable names as they'll appear in your return values/API calls which tends to lead to more readable code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nifty! thanks for the tip!

@inlined inlined assigned laurenzlong and unassigned inlined Oct 23, 2017
@laurenzlong laurenzlong merged commit 0647a82 into master Oct 23, 2017
@laurenzlong laurenzlong deleted the ll-fixfirestoredate branch October 23, 2017 23:30
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.

2 participants