Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attempt to fix locales #2043

Closed
wants to merge 1 commit into from
Closed

Attempt to fix locales #2043

wants to merge 1 commit into from

Conversation

RobbieTheWagner
Copy link
Owner

@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?

@RobbieTheWagner RobbieTheWagner changed the title Import locales Attempt to fix locales Feb 9, 2024
@AmauryD
Copy link
Contributor

AmauryD commented Feb 10, 2024

@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
Copy link
Owner Author

@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
Copy link
Contributor

AmauryD commented Feb 12, 2024

@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
Copy link
Contributor

AmauryD commented Feb 12, 2024

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

@AmauryD
Copy link
Contributor

AmauryD commented Feb 12, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants