Replace dateLib object with DateLib class #2538

daveallie · 2024-10-12T02:12:19Z

What's Changed

dateLib was an object containing a subset of date-fns functions, it is now a full class, which takes that subset and locale/formatting options as constructor arguments.
- Although DateLib can take an undefined Locale, functions within DateLib which different return values based on locale, will require the DateLib class to have been initialised with a locale in order to be called.
  - This should allow for flexibility, while also enforcing the use of locale when it's needed.
- Other formatting options like weekStartsOn, firstWeekContainsDate, useAdditionalWeekYearTokens, useAdditionalDayOfYearTokens are all passed to DateLib on initialization and explicitly not allowed to be passed in directly when calling those functions, instead we use the values that DateLib was initialized with
  - This effectively creates an instance of date-fns with a fixed set of formatting options
I have tried to keep the APIs unchanged, across externally facing functions, using the optional dateLib as it was used before
- dateLib.options exposes the formatting options it was initialized with, allowing for them to be passed down to user-provided label/format functions.

Type of Change

Bug fix
New feature
Breaking change
Documentation update

Additional Notes

Fixes #2509
Related #2523
Related #2511

Include locale, weekStartsOn, firstWeekContainsDate, useAdditionalWeekYearTokens, useAdditionalDayOfYearTokens formatting props in DateLib class

gpbl

Great work here, @daveallie! I’ll need some time to review it. Thanks...

gpbl

I find your solution very interesting, and there's definitely something to learn from your changes. I'm not very up-to-date with OOP in JavaScript, so here are some initial thoughts on DateLib.

The thing is, we should also test it, and I'm trying to find a way to simplify the code.

gpbl · 2024-10-19T16:49:43Z

src/types/props.ts

@@ -437,7 +437,7 @@ export interface PropsBase {
   * @since 9.0.0
   * @experimental
   */
-  dateLib?: Partial<DateLib> | undefined;
+  dateLib?: Partial<DateLibBase> | undefined;


I understand your decision to name the class DateLib and the type DateLibBase, but I find it somewhat confusing. As a consumer, I would expect dateLib to be a partial of a type called DateLib. Additionally, this would introduce a breaking change. ...exploring ways to keep the type name unchanged.

yeah fair, I can leave the name unchanged for exporting, and adjust the internal name from DateLib to DateLibImpl or something

gpbl · 2024-10-19T16:58:33Z

src/lib/dateLib.ts

+   *
+   * @param options The formatting options for the date library.
+   */
+  static fromOptionsDefaultLocale(


Is there a way to avoid using this static method? It’s a bit confusing to me.

yep, let me take another look at my approach, will attempt to rework this part

gpbl · 2024-10-19T17:04:18Z

src/lib/dateLib.ts

+ * @private
+ * @internal
+ */
+export class DateLib {


Would this work somehow?

Suggested change

export class DateLib {

export class DateLib extends DefaultDateLib {

gpbl · 2024-10-19T17:07:05Z

src/lib/dateLib.ts

+    this.dateLibBase = {
+      ...defaultDateLibBase,
+      ...dateLibBaseOverrides
+    };
+  }


Is the dateLibBase property necessary? What about a DateLib class to extends a DefaultDateLib: Then, in the constructor:

if ('addWeeks' in dateLibBaseOverrides) this.addWeeks = dateLibBaseOverrides.addWeeks;

There might be reasons why you’re not doing this, but I can’t identify any just by looking at the code.

currently I've done it this way, as for endOfWeek, format, getWeek, and startOfWeek (locale required functions), I still want access to the original function with the correct name, but the interface from the class to not include the locale and have it pass in the internally known locale to the original function. I'll play around with another way to achieve the same thing a little cleaner.

gpbl · 2024-10-19T17:10:18Z

src/lib/dateLib.ts

+    if (!this.locale) {
+      throw new Error(
+        "DateLib needs to be initialized with locale to use the `format` function."
+      );
+    }
+
+    return format(date, formatStr, this.options);
+  };


The assumption here is that this.options includes this.locale, right? If so, why is this.locale needed separately? Couldn't we just:

Suggested change

if (!this.locale) {

throw new Error(

"DateLib needs to be initialized with locale to use the `format` function."

);

}

return format(date, formatStr, this.options);

};

if (!this.options.locale) {

throw new Error(

"DateLib needs to be initialized with locale to use the `format` function."

);

}

return format(date, formatStr, this.options);

};

P.S. I love how well the code is documented. Thanks!

yeah absolutely! I think what you're suggesting is more clear, I was just using this.locale as shorthand, but it's more indirection.

gpbl · 2024-10-19T17:11:27Z

src/lib/dateLib.ts

+      throw new Error(
+        "DateLib needs to be initialized with locale to use the `getWeek` function."
+      );
+    }
+


Also, can’t we rely on a default this.options.locale to avoid these checks? 🤔

Default locale is what caused the original bug, by making locale optional on deeper calls in the library, we're allowing callers to forget to provide the known locale. For this implementation, I took the approach of still allowing locale optionally, but ensuring it's provided to use any function which is dependent on locale. This should help catch issues where locale is forgotten about in DateLib construction, but required when rendering.

Ultimately DateLib construction only happens inside react-day-picker, so the only time this is hit will be if we've written a bug, it's a footgun prevention mechanism

daveallie · 2024-10-21T04:50:43Z

Thanks for the review @gpbl, I'm not an expert with OOP in JS either tbh, but it's pretty powerful if done right and this seems like a good use for it (wrapping up context and logic into a single instance). I had to wrap my head around the default args in the renderers which were a bit squirrelly, but this approach functions fine and provides a little more robust-ness for future expansion and prevention of issues in the future.

I'll throw up another revision in another few days addressing your feedback, if you spot anything else in there you'd like changed, please call it out. 🫡

gpbl · 2024-10-21T11:04:43Z

@daveallie, I had the chance to review the DateLib class in #2550.

You can check out the code here: dateLib.ts.

I still need to add tests, particularly for the weekday names that you met before, but this version is closer to the concept I had in mind.

If you have some time, I’d appreciate a quick review!

gpbl · 2024-10-21T11:06:47Z

PS. I tried configuring my PR to merge the changes into this one (for an easier diff), but for some reason, I couldn't get it to work...

gpbl · 2024-10-23T10:18:55Z

@daveallie thanks so much for your extensive work here! I've merged #2550 which includes your work here and the test for #2511.

daveallie · 2024-10-23T22:26:19Z

Thanks for picking this up and following it through @gpbl, appreciate it 🤝

daveallie mentioned this pull request Oct 12, 2024

Explicitly provide resolved locale for all functions which need it #2523

Closed

4 tasks

daveallie force-pushed the feature/date-lib-class branch 3 times, most recently from 89feae9 to 9878cf7 Compare October 12, 2024 03:23

daveallie marked this pull request as ready for review October 12, 2024 03:23

gpbl self-requested a review October 12, 2024 10:51

Replace dateLib object with DateLib class

ef6ca8b

Include locale, weekStartsOn, firstWeekContainsDate, useAdditionalWeekYearTokens, useAdditionalDayOfYearTokens formatting props in DateLib class

daveallie force-pushed the feature/date-lib-class branch from 9878cf7 to ef6ca8b Compare October 13, 2024 23:39

Merge branch 'main' into feature/date-lib-class

228420e

gpbl reviewed Oct 19, 2024

View reviewed changes

gpbl mentioned this pull request Oct 21, 2024

feat: DateLib class #2550

Merged

gpbl marked this pull request as draft October 21, 2024 11:04

Merge branch 'main' into feature/date-lib-class

ca857b7

gpbl closed this Oct 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace dateLib object with DateLib class #2538

Replace dateLib object with DateLib class #2538

daveallie commented Oct 12, 2024 •

edited

Loading

gpbl left a comment

gpbl left a comment

gpbl Oct 19, 2024

daveallie Oct 21, 2024

gpbl Oct 19, 2024

daveallie Oct 21, 2024

gpbl Oct 19, 2024

gpbl Oct 19, 2024 •

edited

Loading

daveallie Oct 21, 2024

gpbl Oct 19, 2024

daveallie Oct 21, 2024

gpbl Oct 19, 2024

daveallie Oct 21, 2024

daveallie commented Oct 21, 2024

gpbl commented Oct 21, 2024

gpbl commented Oct 21, 2024 •

edited

Loading

gpbl commented Oct 23, 2024 •

edited

Loading

daveallie commented Oct 23, 2024

	export class DateLib {
	export class DateLib extends DefaultDateLib {

Replace dateLib object with DateLib class #2538

Replace dateLib object with DateLib class #2538

Conversation

daveallie commented Oct 12, 2024 • edited Loading

What's Changed

Type of Change

Additional Notes

gpbl left a comment

Choose a reason for hiding this comment

gpbl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpbl Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daveallie commented Oct 21, 2024

gpbl commented Oct 21, 2024

gpbl commented Oct 21, 2024 • edited Loading

gpbl commented Oct 23, 2024 • edited Loading

daveallie commented Oct 23, 2024

daveallie commented Oct 12, 2024 •

edited

Loading

gpbl Oct 19, 2024 •

edited

Loading

gpbl commented Oct 21, 2024 •

edited

Loading

gpbl commented Oct 23, 2024 •

edited

Loading