h2qutc / angular-material-components

Angular Material Library provide extra components for every project
https://h2qutc.github.io/angular-material-components/
MIT License
331 stars 162 forks source link

Datetime Picker: NgxMatDateAdapter<D> should not assume mutability of D #163

Open rweng opened 3 years ago

rweng commented 3 years ago

Hi there,

thank you for the library!

Regarding the date adapter interface:

https://github.com/h2qutc/angular-material-components/blob/f147c1c9645167fb170bbca4aa9d91f757c2bb5a/projects/datetime-picker/src/lib/core/date-adapter.ts#L90-L98

The interface for the date adapter assumes that the generic date object D is mutable. There are various immutable date implementations like dayjs. I think the interface should implement methods like setHour or setTimeByDefaultValues with a return value to be used as new date object, instead of assuming a mutation of the given parameter.

The default Material Date Adapter works with immutable objects, btw.

What do you think?

CSi-CJ commented 3 years ago

If your custom adapter generic is specified as dayjs, Please refer to the following code: setHour(date: dayjs.Dayjs, value: number): void { date.$H = value; }

setMinute(date: dayjs.Dayjs, value: number): void { date.$m = value; }

setSecond(date: dayjs.Dayjs, value: number): void { date.$s = value; }

rweng commented 3 years ago

@CSi-CJ Thank you, however I have seen this example. I used dayjs as an example but, yeah, you can access the internal properties. However, we ended up using iso strings to avoid converting backend models (i know, using iso strings and converting them is less performant). With strings changing internal properties is not possible (correct me if I'm wrong).

I think that even if you can mutate the properties of dayjs, the pattern to not assume mutability and just return the new instance seems to be better for me (e.g. people might use Object.freeze or something).

murieletrentini commented 2 years ago

I tried the suggested 'solution' for dayjs. however I still cannot change the time value. Is there any other part of the code that assumes mutability?

This is our current implementation:

import { NgxMatDateAdapter } from '@angular-material-components/datetime-picker';
import { Inject, Injectable, InjectionToken, Optional } from '@angular/core';
import { MAT_DATE_LOCALE } from '@angular/material/core';
import * as dayjs from 'dayjs';
import { Dayjs } from 'dayjs';
import 'dayjs/locale/de';
import 'dayjs/locale/en';
import 'dayjs/locale/fr';
import * as customParseFormat from 'dayjs/plugin/customParseFormat';
import * as localeData from 'dayjs/plugin/localeData';
import * as localizedFormat from 'dayjs/plugin/localizedFormat';
import * as utc from 'dayjs/plugin/utc';

/** This file is mostly copied from following git repos but has minor adjustments to handle the time component of dates
 * https://github.com/vanrossumict/material-dayjs-adapter
 * https://github.com/tabuckner/material-dayjs-adapter
 */

export interface DayJsDateAdapterOptions {
  /**
   * Turns the use of utc dates on or off.
   * Changing this will change how Angular Material components like DatePicker output dates.
   * {@default false}
   */
  useUtc?: boolean;
}

export const MAT_DAYJS_DATE_ADAPTER_OPTIONS_FACTORY = (): DayJsDateAdapterOptions => ({
  useUtc: false
});

/** InjectionToken for Dayjs date adapter to configure options. */
export const MAT_DAYJS_DATE_ADAPTER_OPTIONS = new InjectionToken<DayJsDateAdapterOptions>(
  'MAT_DAYJS_DATE_ADAPTER_OPTIONS', {
    providedIn: 'root',
    factory: MAT_DAYJS_DATE_ADAPTER_OPTIONS_FACTORY
  });

/** Creates an array and fills it with values. */
const range = <T>(length: number, valueFunction: (index: number) => T): T[] => {
  const valuesArray = Array(length);
  for (let i = 0; i < length; i++) {
    valuesArray[i] = valueFunction(i);
  }
  return valuesArray;
};

/** Adapts Dayjs Dates for use with Angular Material. */
@Injectable({ providedIn: 'root' })
export class DayjsDateAdapter extends NgxMatDateAdapter<Dayjs> {
  private localeData!: {
    firstDayOfWeek: number;
    longMonths: string[];
    shortMonths: string[];
    dates: string[];
    longDaysOfWeek: string[];
    shortDaysOfWeek: string[];
    narrowDaysOfWeek: string[];
  };

  constructor(@Optional() @Inject(MAT_DATE_LOCALE) public dateLocale: string,
              @Optional() @Inject(MAT_DAYJS_DATE_ADAPTER_OPTIONS) private options?: DayJsDateAdapterOptions) {
    super();

    this.initializeParser(dateLocale);
  }

  setLocale(locale: string) {
    super.setLocale(locale);

    dayjs.locale(locale);

    const dayJsLocaleData = this.dayJs().localeData();
    this.localeData = {
      firstDayOfWeek: dayJsLocaleData.firstDayOfWeek(),
      longMonths: dayJsLocaleData.months(),
      shortMonths: dayJsLocaleData.monthsShort(),
      dates: range(31, (i) => this.createDate(2017, 0, i + 1).format('D')),
      longDaysOfWeek: range(7, (i) => this.dayJs().set('day', i).format('dddd')),
      shortDaysOfWeek: dayJsLocaleData.weekdaysShort(),
      narrowDaysOfWeek: dayJsLocaleData.weekdaysMin(),
    };
  }

  getYear(date: Dayjs): number {
    return this.dayJs(date).year();
  }

  getMonth(date: Dayjs): number {
    return this.dayJs(date).month();
  }

  getDate(date: Dayjs): number {
    return this.dayJs(date).date();
  }

  getDayOfWeek(date: Dayjs): number {
    return this.dayJs(date).day();
  }

  getMonthNames(style: 'long' | 'short' | 'narrow'): string[] {
    return style === 'long' ? this.localeData.longMonths : this.localeData.shortMonths;
  }

  getDateNames(): string[] {
    return this.localeData.dates;
  }

  getDayOfWeekNames(style: 'long' | 'short' | 'narrow'): string[] {
    if (style === 'long') {
      return this.localeData.longDaysOfWeek;
    }
    if (style === 'short') {
      return this.localeData.shortDaysOfWeek;
    }
    return this.localeData.narrowDaysOfWeek;
  }

  getYearName(date: Dayjs): string {
    return this.dayJs(date).format('YYYY');
  }

  getFirstDayOfWeek(): number {
    return this.localeData.firstDayOfWeek;
  }

  getNumDaysInMonth(date: Dayjs): number {
    return this.dayJs(date).daysInMonth();
  }

  getHourNames(): string[] {
    return range(24, (i) => (i === 0 ? '00' : String(i)));
  }

  getHour(date: dayjs.Dayjs): number {
    return this.dayJs(date).hour();
  }

  getMilliseconds(date: dayjs.Dayjs): number {
    return this.dayJs(date).millisecond();
  }

  getMinuteNames(): string[] {
    return range(60, String);
  }

  getMinute(date: dayjs.Dayjs): number {
    return this.dayJs(date).minute();
  }

  getSecond(date: dayjs.Dayjs): number {
    return this.dayJs(date).second();
  }

  setHour(date: dayjs.Dayjs, value: number) {
    // @ts-ignore
    date.$H = value;
  }

  setMinute(date: dayjs.Dayjs, value: number) {
    // @ts-ignore
    date.$m = value;
  }

  setSecond(date: dayjs.Dayjs, value: number) {
    // @ts-ignore
    date.$s = value;
  }

  clone(date: Dayjs): Dayjs {
    return date.clone();
  }

  createDate(year: number, month: number, date: number): Dayjs {
    const returnDayjs = this.dayJs()
      .set('year', year)
      .set('month', month)
      .set('date', date);
    return returnDayjs;
  }

  today(): Dayjs {
    return this.dayJs();
  }

  parse(value: any, parseFormat: string): Dayjs | null {
    if (value && typeof value === 'string') {
      return this.dayJs(value, parseFormat, this.locale);
    }
    return value ? this.dayJs(value).locale(this.locale) : null;
  }

  format(date: Dayjs, displayFormat: string): string {
    if (!this.isValid(date)) {
      throw Error('DayjsDateAdapter: Cannot format invalid date.');
    }
    return date.locale(this.locale).format(displayFormat);
  }

  addCalendarYears(date: Dayjs, years: number): Dayjs {
    return date.add(years, 'year');
  }

  addCalendarMonths(date: Dayjs, months: number): Dayjs {
    return date.add(months, 'month');
  }

  addCalendarDays(date: Dayjs, days: number): Dayjs {
    return date.add(days, 'day');
  }

  addCalendarHours(date: dayjs.Dayjs, hours: number): dayjs.Dayjs {
    return date.add(hours, 'hours');
  }

  addCalendarMinutes(date: dayjs.Dayjs, minutes: number): dayjs.Dayjs {
    return date.add(minutes, 'minutes');
  }

  addCalendarSeconds(date: dayjs.Dayjs, seconds: number, ms: number | undefined): dayjs.Dayjs {
    const newDate = date.add(seconds, 'seconds');
    return ms === undefined
      ? newDate
      : newDate.add(ms, 'milliseconds');
  }

  toIso8601(date: Dayjs): string {
    return date.toISOString();
  }

  /**
   * Attempts to deserialize a value to a valid date object. This is different from parsing in that
   * deserialize should only accept non-ambiguous, locale-independent formats (e.g. a ISO 8601
   * string). The default implementation does not allow any deserialization, it simply checks that
   * the given value is already a valid date object or null. The `<mat-datepicker>` will call this
   * method on all of it's `@Input()` properties that accept dates. It is therefore possible to
   * support passing values from your backend directly to these properties by overriding this method
   * to also deserialize the format used by your backend.
   *
   * @param value The value to be deserialized into a date object.
   * @returns The deserialized date object, either a valid date, null if the value can be
   *     deserialized into a null date (e.g. the empty string), or an invalid date.
   */
  deserialize(value: any): Dayjs | null {
    let date: any;
    if (value instanceof Date) {
      date = this.dayJs(value);
    } else if (this.isDateInstance(value)) {
      // NOTE: assumes that cloning also sets the correct locale.
      return this.clone(value);
    }
    if (typeof value === 'string') {
      if (!value) {
        return null;
      }
      date = this.dayJs(value).toISOString();
    }
    if (date && this.isValid(date)) {
      return this.dayJs(date); // NOTE: Is this necessary since Dayjs is immutable and Moment was not?
    }
    return super.deserialize(value);
  }

  isDateInstance(obj: any): boolean {
    return dayjs.isDayjs(obj);
  }

  isValid(date: Dayjs): boolean {
    return this.dayJs(date).isValid();
  }

  invalid(): Dayjs {
    return this.dayJs(null);
  }

  private dayJs(input?: any, format?: string, locale?: string): Dayjs {
    if (this.shouldUseUtc) {
      return dayjs(input, format, locale).utc();
    } else {
      return dayjs(input, format, locale);
    }
  }

  private get shouldUseUtc(): boolean {
    const { useUtc }: DayJsDateAdapterOptions = this.options || {};
    return !!useUtc;
  }

  private initializeParser(dateLocale: string) {
    if (this.shouldUseUtc) {
      dayjs.extend(utc);
    }

    dayjs.extend(localizedFormat);
    dayjs.extend(customParseFormat);
    dayjs.extend(localeData);

    this.setLocale(dateLocale);
  }
}
kzander91 commented 2 years ago

I stumbled upon this issue as well when trying to implement a js-joda adapter using Instant as the model type. I worked around this by modifying the internal state of my Instant instances like so:

setHour(date: Instant, value: number): void {
  let modified = date.atZone(...).withHour(value).toInstant();
  // @ts-ignore
  date._seconds = modified._seconds;
  // @ts-ignore
  date._nanos = modified._nanos;
}
// repeat for setMinute() and setSecond()

This seems rather dirty, native support for immutable model types would be appreciated.

gridwire commented 1 year ago

Hi @murieletrentini,

It's ugly but..

As you already use this.dayJs you could explicitly return a clone (you already have the clone function). Basically, leave the original immutable and change the copy for every change you make. With setHour for instance, you can use the date object directly like this:

setHour(date: Dayjs, value: number): void { date.set("hours", value); }

This won't work unless you add the final step:

import badMutable from "dayjs/plugin/badMutable"; dayjs.extend(badMutable);

No it should work I guess, I see no other way.