Skip to content
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

[1.x] [Labs] Add useManualCalc prop to TimezonePicker as speed optimization #2564

Merged
merged 4 commits into from
Jun 11, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,13 @@ export function formatTimezone(
timezone: string | undefined,
date: Date,
displayFormat: TimezoneDisplayFormat,
useManualCalc: boolean,
): string | undefined {
if (!timezone || !moment.tz.zone(timezone)) {
return undefined;
}

const { abbreviation, offsetAsString } = getTimezoneMetadata(timezone, date);
const { abbreviation, offsetAsString } = getTimezoneMetadata(timezone, date, useManualCalc);
switch (displayFormat) {
case TimezoneDisplayFormat.ABBREVIATION:
// Fall back to the offset when there is no abbreviation.
Expand Down
30 changes: 16 additions & 14 deletions packages/labs/src/components/timezone-picker/timezoneItems.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ export interface ITimezoneItem {
* Get a list of all timezone items.
* @param date the date to use when determining timezone offsets
*/
export function getTimezoneItems(date: Date): ITimezoneItem[] {
return moment.tz.names().map(timezone => toTimezoneItem(timezone, date));
export function getTimezoneItems(date: Date, useManualCalc: boolean): ITimezoneItem[] {
return moment.tz.names().map(timezone => toTimezoneItem(timezone, date, useManualCalc));
}

/**
Expand All @@ -42,22 +42,24 @@ export function getTimezoneItems(date: Date): ITimezoneItem[] {
* @param date the date to use when determining timezone offsets
* @param includeLocalTimezone whether to include the local timezone
*/
export function getInitialTimezoneItems(date: Date, includeLocalTimezone: boolean): ITimezoneItem[] {
const populous = getPopulousTimezoneItems(date);
const local = getLocalTimezoneItem(date);
export function getInitialTimezoneItems(
date: Date,
includeLocalTimezone: boolean,
useManualCalc: boolean,
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth setting useManualCalc: boolean = false? Then you don't have to update all those tests with the extra param.

): ITimezoneItem[] {
const populous = getPopulousTimezoneItems(date, useManualCalc);
const local = getLocalTimezoneItem(date, useManualCalc);
return includeLocalTimezone && local !== undefined ? [local, ...populous] : populous;
}

/**
* Get the timezone item for the user's local timezone.
* @param date the date to use when determining timezone offsets
*/
export function getLocalTimezoneItem(date: Date): ITimezoneItem | undefined {
export function getLocalTimezoneItem(date: Date, useManualCalc: boolean): ITimezoneItem | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

const timezone = getLocalTimezone();
if (timezone !== undefined) {
const timestamp = date.getTime();
const zonedDate = moment.tz(timestamp, timezone);
const offsetAsString = zonedDate.format("Z");
const { offsetAsString } = getTimezoneMetadata(timezone, date, useManualCalc);
return {
iconName: "locate",
key: `${timezone}-local`,
Expand All @@ -75,13 +77,13 @@ export function getLocalTimezoneItem(date: Date): ITimezoneItem | undefined {
* than one region for the offset.
* @param date the date to use when determining timezone offsets
*/
function getPopulousTimezoneItems(date: Date): ITimezoneItem[] {
function getPopulousTimezoneItems(date: Date, useManualCalc: boolean): ITimezoneItem[] {
// Filter out noisy timezones. See https://github.com/moment/moment-timezone/issues/227
const timezones = moment.tz.names().filter(timezone => /\//.test(timezone) && !/Etc\//.test(timezone));

const timezoneToMetadata: { [timezone: string]: ITimezoneMetadata } = {};
for (const timezone of timezones) {
timezoneToMetadata[timezone] = getTimezoneMetadata(timezone, date);
timezoneToMetadata[timezone] = getTimezoneMetadata(timezone, date, useManualCalc);
}

// Order by offset ascending, population descending, timezone name ascending
Expand All @@ -106,15 +108,15 @@ function getPopulousTimezoneItems(date: Date): ITimezoneItem[] {
for (const timezone of timezones) {
const curOffset = timezoneToMetadata[timezone].offset;
if (prevOffset === undefined || prevOffset !== curOffset) {
initialTimezones.push(toTimezoneItem(timezone, date));
initialTimezones.push(toTimezoneItem(timezone, date, useManualCalc));
prevOffset = curOffset;
}
}
return initialTimezones;
}

function toTimezoneItem(timezone: string, date: Date): ITimezoneItem {
const { abbreviation, offsetAsString } = getTimezoneMetadata(timezone, date);
function toTimezoneItem(timezone: string, date: Date, useManualCalc: boolean): ITimezoneItem {
const { abbreviation, offsetAsString } = getTimezoneMetadata(timezone, date, useManualCalc);
return {
key: timezone,
label: offsetAsString,
Expand Down
71 changes: 55 additions & 16 deletions packages/labs/src/components/timezone-picker/timezoneMetadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,33 @@ export interface ITimezoneMetadata {
population: number | undefined;
}

export function getTimezoneMetadata(timezone: string, date: Date): ITimezoneMetadata {
const timestamp = date.getTime();
export function getTimezoneMetadata(timezone: string, date: Date, useManualCalc: boolean): ITimezoneMetadata {
const zone = moment.tz.zone(timezone);
const timestamp = date.getTime();
const calculateMetadata = useManualCalc ? getMetadataFromMomentManual : getMetadataFromMoment;
return calculateMetadata(timezone, zone, timestamp);
}

/**
* Ignore abbreviations that are simply offsets, i.e. "+14" instead of "PST"
* @param abbreviation
*/
function getNonOffsetAbbreviation(abbreviation: string) {
return isNonOffsetAbbreviation(abbreviation) ? abbreviation : undefined;
}

function isNonOffsetAbbreviation(abbreviation: string) {
return abbreviation != null && abbreviation.length > 0 && abbreviation[0] !== "-" && abbreviation[0] !== "+";
}

/**
* Use moment-timezone to parse the timestamp and provide timezone metadata
*/
function getMetadataFromMoment(timezone: string, zone: moment.MomentZone, timestamp: number) {
const zonedDate = moment.tz(timestamp, timezone);
const offset = zonedDate.utcOffset();
const offsetAsString = zonedDate.format("Z");
const abbreviation = getAbbreviation(timezone, timestamp);

const abbreviation = getNonOffsetAbbreviation(zonedDate.zoneAbbr());
return {
abbreviation,
offset,
Expand All @@ -32,21 +51,41 @@ export function getTimezoneMetadata(timezone: string, date: Date): ITimezoneMeta
}

/**
* Get the abbreviation for a timezone.
* We need this utility because moment-timezone's `abbr` will not always give the abbreviated time zone name,
* since it falls back to the time offsets for each region.
* https://momentjs.com/timezone/docs/#/using-timezones/formatting/
* Manually determine timezone metadata by skipping the timestamp parsing and following
* http://momentjs.com/timezone/docs/#/data-formats/unpacked-format/
*/
function getAbbreviation(timezone: string, timestamp: number): string | undefined {
const zone = moment.tz.zone(timezone);
if (zone) {
const abbreviation = zone.abbr(timestamp);
function getMetadataFromMomentManual(timezone: string, zone: moment.MomentZone, timestamp: number) {
const { abbrs, offsets, population, untils } = zone;
const index = findOffsetIndex(timestamp, untils);
const abbreviation = getNonOffsetAbbreviation(abbrs[index]);
const offset = offsets[index] * -1;
const offsetAsString = getOffsetAsString(offset);
return {
abbreviation,
offset,
offsetAsString,
population,
timezone,
};
}

// Only include abbreviations that are not just a repeat of the offset
if (abbreviation.length > 0 && abbreviation[0] !== "-" && abbreviation[0] !== "+") {
return abbreviation;
function findOffsetIndex(timestamp: number, untils: number[]) {
for (let i = 0; i < untils.length; i++) {
if (i === untils.length - 1 || timestamp < untils[i]) {
return i;
}
}
return 0;
}

function getOffsetAsString(offset: number) {
const offsetVal = Math.abs(offset);
const minutes = offsetVal % 60;
const hours = (offsetVal - minutes) / 60;
const sign = offset >= 0 ? "+" : "-";
return sign + lpad(hours) + ":" + lpad(minutes);
Copy link
Contributor

Choose a reason for hiding this comment

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

slight preference for template string.

}

return undefined;
function lpad(num: number) {
return num < 10 ? "0" + num : num;
}
29 changes: 21 additions & 8 deletions packages/labs/src/components/timezone-picker/timezonePicker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ export interface ITimezonePickerProps extends IProps {
*/
showLocalTimezone?: boolean;

/**
* Whether to use manual calculation (faster, but possibly less accurate) rather than moment-timezone to infer metadata;
Copy link
Contributor

Choose a reason for hiding this comment

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

  • typo: Add trailing period, not semicolon.
  • Also, can you add language clarifying what is less accurate when useManualCalc=true?

* @default false
*/
useManualCalc?: boolean;

/**
* Format to use when displaying the selected (or default) timezone within the target element.
* @default TimezoneDisplayFormat.OFFSET
Expand Down Expand Up @@ -111,6 +117,7 @@ export class TimezonePicker extends AbstractComponent<ITimezonePickerProps, ITim
placeholder: "Select timezone...",
popoverProps: {},
showLocalTimezone: true,
useManualCalc: false,
};

private timezoneItems: ITimezoneItem[];
Expand All @@ -119,12 +126,12 @@ export class TimezonePicker extends AbstractComponent<ITimezonePickerProps, ITim
constructor(props: ITimezonePickerProps, context?: any) {
super(props, context);

const { value, date = new Date(), showLocalTimezone, inputProps = {} } = props;
const { value, date = new Date(), useManualCalc, showLocalTimezone, inputProps = {} } = props;
const query = inputProps.value !== undefined ? inputProps.value : "";
this.state = { date, value, query };

this.timezoneItems = getTimezoneItems(date);
this.initialTimezoneItems = getInitialTimezoneItems(date, showLocalTimezone);
this.timezoneItems = getTimezoneItems(date, useManualCalc);
this.initialTimezoneItems = getInitialTimezoneItems(date, showLocalTimezone, useManualCalc);
}

public render() {
Expand Down Expand Up @@ -161,14 +168,14 @@ export class TimezonePicker extends AbstractComponent<ITimezonePickerProps, ITim
}

public componentWillReceiveProps(nextProps: ITimezonePickerProps) {
const { date: nextDate = new Date(), inputProps: nextInputProps = {} } = nextProps;
const { date: nextDate = new Date(), inputProps: nextInputProps = {}, useManualCalc } = nextProps;
const dateChanged = this.state.date.getTime() !== nextDate.getTime();

if (dateChanged) {
this.timezoneItems = getTimezoneItems(nextDate);
this.timezoneItems = getTimezoneItems(nextDate, useManualCalc);
}
if (dateChanged || this.props.showLocalTimezone !== nextProps.showLocalTimezone) {
this.initialTimezoneItems = getInitialTimezoneItems(nextDate, nextProps.showLocalTimezone);
this.initialTimezoneItems = getInitialTimezoneItems(nextDate, nextProps.showLocalTimezone, useManualCalc);
}

const nextState: ITimezonePickerState = {};
Expand All @@ -187,6 +194,7 @@ export class TimezonePicker extends AbstractComponent<ITimezonePickerProps, ITim
private renderButton() {
const {
disabled,
useManualCalc,
valueDisplayFormat = TimezoneDisplayFormat.OFFSET,
defaultValue,
placeholder,
Expand All @@ -195,7 +203,9 @@ export class TimezonePicker extends AbstractComponent<ITimezonePickerProps, ITim
const { date, value } = this.state;

const finalValue = value ? value : defaultValue;
const displayValue = finalValue ? formatTimezone(finalValue, date, valueDisplayFormat) : undefined;
const displayValue = finalValue
? formatTimezone(finalValue, date, valueDisplayFormat, useManualCalc)
: undefined;

return (
<Button
Expand All @@ -212,8 +222,11 @@ export class TimezonePicker extends AbstractComponent<ITimezonePickerProps, ITim
return items;
}

const { useManualCalc } = this.props;
const { date } = this.state;
return filterWithQueryCandidates(items, query, item => getTimezoneQueryCandidates(item.timezone, date));
return filterWithQueryCandidates(items, query, item =>
getTimezoneQueryCandidates(item.timezone, date, useManualCalc),
);
};

private renderItem = (itemProps: ISelectItemRendererProps<ITimezoneItem>) => {
Expand Down
4 changes: 2 additions & 2 deletions packages/labs/src/components/timezone-picker/timezoneUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ export function getLocalTimezone(): string | undefined {
* @param date the date to use when determining timezone offsets
* @returns a list of queryable strings
*/
export function getTimezoneQueryCandidates(timezone: string, date: Date): string[] {
const { abbreviation, offsetAsString } = getTimezoneMetadata(timezone, date);
export function getTimezoneQueryCandidates(timezone: string, date: Date, useManualCalc: boolean): string[] {
const { abbreviation, offsetAsString } = getTimezoneMetadata(timezone, date, useManualCalc);
return [timezone, abbreviation, offsetAsString].filter(candidate => candidate !== undefined);
}

Expand Down
13 changes: 9 additions & 4 deletions packages/labs/test/timezonePickerTests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ describe("<TimezonePicker>", () => {
const date = new Date();
const timezonePicker = shallow(<TimezonePicker date={date} popoverProps={getPopoverProps()} />);
const items = findSelect(timezonePicker).prop("items");
assert.deepEqual(items, getInitialTimezoneItems(date, true));
assert.deepEqual(items, getInitialTimezoneItems(date, true, false));
});

it("if inputProps.value is non-empty, all items are shown", () => {
Expand All @@ -85,7 +85,7 @@ describe("<TimezonePicker>", () => {
);
assert.strictEqual(timezonePicker.state("query"), query);
const items = findSelect(timezonePicker).prop("items");
assert.deepEqual(items, getTimezoneItems(date));
assert.deepEqual(items, getTimezoneItems(date, false));
});

it("if inputProps.value is non-empty and it changes to a different non-empty value, the same items are shown", () => {
Expand Down Expand Up @@ -129,7 +129,7 @@ describe("<TimezonePicker>", () => {
const items = findSelect(timezonePicker).prop("items");
assert.isTrue(items.length > 0);
const firstItem = items[0];
const expectedFirstItem = getInitialTimezoneItems(date, false)[0];
const expectedFirstItem = getInitialTimezoneItems(date, false, false)[0];
assert.deepEqual(firstItem, expectedFirstItem);
});

Expand All @@ -140,7 +140,7 @@ describe("<TimezonePicker>", () => {
<TimezonePicker date={date} inputProps={{ value: query }} popoverProps={getPopoverProps()} />,
);
const items = findSelect(timezonePicker).prop("items");
const localTimezoneItem = getLocalTimezoneItem(date);
const localTimezoneItem = getLocalTimezoneItem(date, false);
const itemsWithLocalTimezone = items.filter(item => item.timezone === localTimezoneItem.timezone);
for (const item of itemsWithLocalTimezone) {
assert.notDeepEqual(item, localTimezoneItem);
Expand Down Expand Up @@ -209,6 +209,11 @@ describe("<TimezonePicker>", () => {
});
});

it("manual metadata matches moment metadata", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👌

Would be helpful to have a test where the two aren't equal (i.e. a "less accurate" case).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cmslewis had a discussions with @giladgray about possibly removing the forking logic and doing a examination about whether or not it is actually less accurate.

Specifically, I actually don't know of any case where they aren't equal but have not run a comprehensive test checking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cmslewis @giladgray wrote up a fiddle here to test, with { +1, 0, +1 } X untils per timezone and tested against all timezones

http://jsfiddle.net/9hnsLgx6/5/

Looks like there are 2338 unique dates, 583 timezones, 1362471 matches, 0 misses.

If this looks good to just replace the logic, I can remove the forking behavior?

const date = new Date();
assert.deepEqual(getTimezoneItems(date, true), getTimezoneItems(date, false));
});

it("invokes the onChange input prop", () => {
const query = "test query";
const onInputChange = sinon.spy();
Expand Down