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

Dynamically import locales #2047

Closed AmauryD closed 7 months ago

AmauryD commented 7 months ago

@RobbieTheWagner The only downside of the initializer is that it always import the core of the flatpickr library. Which is fine most of the time because the core is imported when we use the component in the app at least once. My only concern is when route splitting is enabled. The library will probably always be imported even if not used in the route. In this case i could create a @embroider/macro configuration flag that disable the initializer import when requested.

RobbieTheWagner commented 7 months ago

@RobbieTheWagner The only downside of the initializer is that it always import the core of the flatpickr library. Which is fine most of the time because the core is imported when we use the component in the app at least once. My only concern is when route splitting is enabled. The library will probably always be imported even if not used in the route. In this case i could create a @embroider/macro configuration flag that disable the initializer import when requested.

I would love to find a way to make things work without needing to force flatpickr onto the window so early. I admit, I still don't understand why we need it on the window for locales to work. I would expect importing the locale and then passing what you imported in to work.

AmauryD commented 7 months ago

I would love to find a way to make things work without needing to force flatpickr onto the window so early. I admit, I still don't understand why we need it on the window for locales to work. I would expect importing the locale and then passing what you imported in to work.

Me too, it was working before because the lib was put in vendor.js that was loaded at the start of the app. https://github.com/RobbieTheWagner/ember-flatpickr/blob/93d73ca7d0f501c68702730b708f2f0eca321ed1/index.js.

For the explanation, i'll try to make an example repo for a clearer explanation of the problem. My english is sometimes not the best. And flatpickr has a weird initialization logic.

I'll do some research on my side.

AmauryD commented 7 months ago

I've made an example repo with console.logs to see the problem. https://github.com/AmauryD/demo-flapickbug

image.

To be able to register a locale globaly, flatpickr must be registed before the locale file imported. Otherwise, the locale is never registered to the global window.flatpickr.l10ns. The problem is that ember calls the import 'flatpickr' after you import the locale. That moment is when the EmberFlapickr component is invoked. So the locale is never registered to the global object and can't be resolved using the string syntax.

I know it's a weird behavior but that's how flatpickr deals with locales.

Not sure if the explanation is better.

AmauryD commented 7 months ago

I added an option to disable this behavior. So it can be manually disabled and then tree-shaked if needed. What do you think ?

https://github.com/RobbieTheWagner/ember-flatpickr/blob/91bbe64114adbb24293c9e58b90b8bdac7a5b06b/ember-flatpickr/src/initializers/ember-flatpickr.ts#L4-L13

Will need a glass of whiskey after this

AmauryD commented 7 months ago

The merge from main broke the tests. I will take a look tomorrow

RobbieTheWagner commented 7 months ago

To be able to register a locale globaly, flatpickr must be registed before the locale file imported

What if the ember-flatpickr component took locale="fr" for example and then we handled the logic inside the component to dynamically import the locale files? At that point, the component should have already been instantiated and the flatpickr reference should exist.

AmauryD commented 7 months ago

To be able to register a locale globaly, flatpickr must be registed before the locale file imported

What if the ember-flatpickr component took locale="fr" for example and then we handled the logic inside the component to dynamically import the locale files? At that point, the component should have already been instantiated and the flatpickr reference should exist.

That's a possibility, but i don't know if the tree shaking will work if you dynamically import the locale based on runtime variables. Will try this later.

AmauryD commented 7 months ago

It looks like if i do something like this: (Just a test code, not the final result)


  async setupFlatpickr(element: HTMLInputElement): Promise<void> {
    const { date, onChange, wrap, locale } = this.args;

    // Require that users pass a date
    assert(
      '<EmberFlatpickr> requires a `date` to be passed as the value for flatpickr.',
      date !== undefined,
    );

    // Require that users pass an onChange
    assert(
      '<EmberFlatpickr> requires an `onChange` action or null for no action.',
      onChange !== undefined,
    );

    // Wrap is not supported
    assert(
      '<EmberFlatpickr> does not support the wrap option. Please see documentation for an alternative.',
      wrap !== true,
    );

    if (locale) {
      await import(`flatpickr/dist/l10n/${locale}`);
    }

    // Pass all values and setup flatpickr
    scheduleOnce('afterRender', this, this._setFlatpickrOptions, element);
  }

It works. Webpack generates a bundle for each lang. These are lazy-loaded later. Great ! These bundlers are very smart.

image

All the small red squares are a locale.

image

When i choose a lang that is not already loaded it calls the chunk.

RobbieTheWagner commented 7 months ago

Great, glad that dynamic importing seems to work! Let's do that then, and not add an initializer.

AmauryD commented 7 months ago

Just a side note, but i tried to upgrade eslint-plugin-ember and there are some deprecations concerning using @ember/runloop and 'afterRender' queue.

AmauryD commented 7 months ago

No problem, thank's for your support !