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.

Sign up for GitHub

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(datetime)!: get day of week key without using locale #23

Merged
merged 1 commit into from
Jan 21, 2025
+20 −30
Merged

fix(datetime)!: get day of week key without using locale #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions src/helpers/datetime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,15 @@ export const localizeDate = (dateObj: Date, locale: string, format: localizeDate

/**
* Gets the appropriate 3 letter key for the day of the week for a given date object
* used to find the correct key of the hour data to pull data off of
* used to find the correct key of the hour data to pull data off of.
* We localize using the `en-US` locale, because the date keys are in that locale.
*
* @param {Date} dateObj
* @param {locale} string - Hyphenized language/country locale combo, e.g. en-US (BCP 47).
* @param {String} timezone
* @return {DayOfWeek}
*/
export const getDayOfWeekKey = (dateObj: Date, locale: string, timezone: string): DayOfWeek => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a breaking change since we're changing the param signature, are any of these methods public to the developer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we do expose a couple methods whose param signatures are changing, yeah: getNextOpenIntervalToday and getOpenIntervalsForToday (although we're not using them in Mise)

We could fix the bug while keeping backwards compatibility, by keeping the locale parameter there and simply not using it, marking it as deprecated etc. I don't know if that's worth it though tbh. We could investigate

@kevinlee11 how are we handling breaking changes in the SDK at the moment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's not being used by Mise yet, it's likely safe to make a breaking change because we never documented any of these helpers. Once we document them and/or start using it in Mise I'd have said we should avoid breaking changes for the external theme devs' sake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kk. are we tracking breaking changes in the semver for Alpha versions, or no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, but we haven't had any breaking changes to date.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should still mark this as a breaking change and get into the habit of it since this generates release notes (see https://github.com/square/site-theme-sdk/releases/tag/v1.0.0-alpha.11), while some of our internal themes may not use it i think we should still run through the process

This repo uses convention commits so it takes the guess work out of humans making up version numbers and its just based off of what is changed in the code

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, but we haven't had any breaking changes to date.

we actually did just release a breaking change last release, it's fine because it's just Alpha right now so it won't actually bump the major value

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh right, forgot about the orders removal

Copy link
Collaborator

Choose a reason for hiding this comment

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

agreed, we should still have the release notes cover the breaking change, but like Kevin said, it will just incrementally update the version because it's in alpha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've rebased this with Conventional Commit messages, and marked it as a breaking change

const dayOfWeek = localizeDate(dateObj, locale, DateFormats.weekdayShort, timezone);
export const getDayOfWeekKey = (dateObj: Date, timezone: string): DayOfWeek => {
const dayOfWeek = localizeDate(dateObj, 'en-US', DateFormats.weekdayShort, timezone);
return dayOfWeek.toUpperCase() as DayOfWeek;
};

Expand Down
13 changes: 3 additions & 10 deletions src/helpers/location.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ export class Location {
};
}

const nextOpenIntervalAfterToday = this.getNextOpenIntervalAfterToday(locale, timezone, hours);
const nextOpenIntervalAfterToday = this.getNextOpenIntervalAfterToday(timezone, hours);
if (nextOpenIntervalAfterToday) {
const { date, interval: nextInterval } = nextOpenIntervalAfterToday;
const opensAt = getFormattedTime(date, nextInterval.open, DateFormats.hourNminuteN, locale, timezone, tzOffsetString);
Expand Down Expand Up @@ -173,7 +173,6 @@ export class Location {
nextDateObj.setDate(nextDateObj.getDate() + 1);
const dayOfWeekKey = getDayOfWeekKey(
nextDateObj,
locale,
timezone,
);

Expand Down Expand Up @@ -217,19 +216,16 @@ export class Location {
/**
* Gets open intervals for today
*
* @param {string} locale - Hyphenized language/country locale combo, e.g. en-US (BCP 47).
* @param {string} timezone
* @param {Hours} hours
* @return {OpenInterval[]}
*/
getOpenIntervalsForToday(
locale: string,
timezone: string,
hours: Hours
): OpenInterval[] {
const dayOfWeekKey: DayOfWeek = getDayOfWeekKey(
new Date(),
locale,
timezone
);
return hours[dayOfWeekKey];
Expand All @@ -248,7 +244,7 @@ export class Location {
timezone: string,
hours: Hours
): OpenInterval | null {
const todayOpenIntervals = this.getOpenIntervalsForToday(locale, timezone, hours);
const todayOpenIntervals = this.getOpenIntervalsForToday(timezone, hours);
if (!todayOpenIntervals.length) {
return null;
}
Expand Down Expand Up @@ -282,7 +278,7 @@ export class Location {
timezone: string,
hours: Hours
): OpenInterval | null {
const todayOpenIntervals = this.getOpenIntervalsForToday(locale, timezone, hours);
const todayOpenIntervals = this.getOpenIntervalsForToday(timezone, hours);
if (!todayOpenIntervals.length) {
return null;
}
Expand Down Expand Up @@ -311,13 +307,11 @@ export class Location {
/**
* Get the next opening period after today within the next seven days
*
* @param {string} locale - Hyphenized language/country locale combo, e.g. en-US (BCP 47).
* @param {string} timezone
* @param {Hours} hours
* @return {Object | null} { open: openTime, close: closeTime }
*/
getNextOpenIntervalAfterToday(
locale: string,
timezone: string,
hours: Hours,
): {
Expand All @@ -331,7 +325,6 @@ export class Location {
nextDateObj.setDate(nextDateObj.getDate() + 1);
const dayOfWeekKey = getDayOfWeekKey(
nextDateObj,
locale,
timezone,
);

Expand Down
1 change: 0 additions & 1 deletion test/helpers.location.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -982,7 +982,6 @@ describe('Check open intervals for today', () => {
vi.setSystemTime(date);

const result = sdk.helpers.location.getOpenIntervalsForToday(
'en-US',
'UTC',
location['PICKUP'].hours
);
Expand Down
22 changes: 10 additions & 12 deletions typedocs/classes/helpers_location.Location.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions typedocs/modules/helpers_datetime.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading