fix(datetime)!: get day of week key without using locale #23
Conversation
bashunaimiroy
changed the title
size-limit report 📦
|
📊 Package size report -0.39%↓
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
| * @param {String} timezone | ||
| * @return {DayOfWeek} | ||
| */ | ||
| export const getDayOfWeekKey = (dateObj: Date, locale: string, timezone: string): DayOfWeek => { |
There was a problem hiding this comment.
This looks like a breaking change since we're changing the param signature, are any of these methods public to the developer?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
kk. are we tracking breaking changes in the semver for Alpha versions, or no?
There was a problem hiding this comment.
Nope, but we haven't had any breaking changes to date.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
oh right, forgot about the orders removal
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I've rebased this with Conventional Commit messages, and marked it as a breaking change
bashunaimiroy
force-pushed
the
week-key-no-localize
branch
from
bca7bc1 to
a2ff2d9
Compare
note that this removes the "locale" parameter from the param signature of several methods: * helpers.datetime.getDayOfWeekKey * helpers.location.getNextOpenIntervalAfterToday * helpers.location.getOpenIntervalsForToday
bashunaimiroy
force-pushed
the
week-key-no-localize
branch
from
a2ff2d9 to
d77f075
Compare
bashunaimiroy
changed the title
|
@kchung @kevinlee11 just need øne more approval on this |
Bug
When requesting store hours for the current day, and the Locale Language is other than English, the SDK throws an error
Cause
The days of the week are not localised in Square Data; we are fetching them by generating a localised key according to the current day
Fix
don't localize the key when we are accessing that data.