RobbieTheWagner / ember-flatpickr

An Ember addon that wraps the Flatpickr date picker
https://RobbieTheWagner.github.io/ember-flatpickr/
MIT License
109 stars 55 forks source link

Attempt to fix locales #2043

Closed RobbieTheWagner closed 7 months ago

RobbieTheWagner commented 8 months ago

@AmauryD are you able to get locales to work? I noticed they were not working when changing them in the docs. Can you take a look?

AmauryD commented 8 months ago

@RobbieTheWagner Using locales as strings seems a bit tricky now with embroider.

It looks like a race condition to me. The code executed when a locale is imported looks like this :

const fp =
  typeof window !== "undefined" && window.flatpickr !== undefined
    ? window.flatpickr
    : ({
        l10ns: {},
      } as FlatpickrFn);

The locale is registered to the flatpickr.l10ns object and then usable as string. The problem is, at "import time". The window.flatpickr instance does not seems to exists (the flatpickr component is probably executed later). So the locale is read but no locale is registered globally.

Creating an initializer that initializes the window.flatpickr instance upstream solves the problem. Or an import in router.js with this code:

import flatpickr from 'flatpickr';

window.flatpickr = flatpickr;

Apologies, i didn't notice using string for locale was broken during the migration to V2 format.

Should we document this or should we create an initializer to handle this case ?

RobbieTheWagner commented 7 months ago

@AmauryD If we made an initializer, wouldn't that mean flatpickr would always be included and we would lose the benefits of tree shaking and such? FWIW, I also tried importing specific locales, not using the strings, and it was still saying like [object Object] not a valid locale or whatever for those.

I do think the whole point of an addon is to make things as easy as possible for the users of the addon, so if that means we need an initializer, maybe that is the best option.

AmauryD commented 7 months ago

@AmauryD If we made an initializer, wouldn't that mean flatpickr would always be included and we would lose the benefits of tree shaking and such?

I don't think so, the main entrypoint of flatpickr does not import the locales (only english as default). So they are "tree-shaked" away. I will verify this statement later using a bundle analyzer.

FWIW, I also tried importing specific locales, not using the strings, and it was still saying like [object Object] not a valid locale or whatever for those.

Importing the locale this way works :

import Controller from '@ember/controller';
import { action } from '@ember/object';
import { tracked } from '@glimmer/tracking';
import { next } from '@ember/runloop';
import de from 'flatpickr/dist/l10n/de';
import fr from 'flatpickr/dist/l10n/fr';
import ru from 'flatpickr/dist/l10n/ru';
import uk from 'flatpickr/dist/l10n/uk';

export default class EmberFlatpickr extends Controller {
...

  get locales() {
    return ['default', fr.fr, de.de, ru.ru, uk.uk];
  }
  ...
}

Just need to fix the [Object Object] in select.

I do think the whole point of an addon is to make things as easy as possible for the users of the addon, so if that means we need an initializer, maybe that is the best option.

I agree on this.

AmauryD commented 7 months ago

I will take care of the issue today if you want.

AmauryD commented 7 months ago

Testing something in #2047 . I can confirm the locales are not imported by default. You can see by uncommenting the bundle analyzer in my PR.