arthurdenner / react-semantic-ui-datepickers

Datepickers built with Semantic UI for React and Dayzed.
https://react-semantic-ui-datepickers.vercel.app
MIT License
95 stars 56 forks source link

Locales can't be found on ESM module and CodeSandbox #176

Open VoxValue opened 4 years ago

VoxValue commented 4 years ago

❔ Question

Hi

I just made the upgrade from the previous version. So, I replace the locale import by a reference to locale into the tag definition (as following): <SemanticDatepicker selected={oDateRange} type="range" showOutsideDays locale="fr-FR"...>

The build step (based on rollup) runs correctly, but the execution fails with the following error: ReferenceError: "require is not defined" More specifically, at: react-semantic-ui-datepickers.esm.js:1084.

I import a lot of esm files, withount encounter this issue. Moreover, according the rollup configuration, it seems the locales files must be embedded into the distribution files. But, it seems do not be the case.

Has I missed some thing ?

Thanks in advance

Fred

arthurdenner commented 4 years ago

Hi @Fred2MTL! 👋

The locales folder is published as you can see on unpkg.

However, I've tried to create an example on CodeSandbox and it's displaying only the en-US.json file and I don't understand why. 🤔

image

Regarding your error, require is not defined, I'm not sure what could be the issue as this works on Storybook locally and on the website. 😕

VoxValue commented 4 years ago

@arthurdenner

I understand 'require' in this context as a loading command specific to the node context, not to the library. Beacause:

So, in my undestanding, it must be resolved at the build time (e.g by rollup) by including the locales directory into the distributed library. Moreover, assuming it forces the locales files to be in this directory, it could not be extended at the design time / the runtime by providing another locales structure.

Concerning the storybook point and without deep knowledge, I think the code is run on the server side (e.g. into a node context), explaining the effective load of locales by node, prior to any transformations into Httml / JS, not the client side.

Fred

arthurdenner commented 4 years ago

But the locales folder is in the dist folder as you can see in unpkg link. I'm going to tag this and feel free to work on it if you want, I'm quite busy with a personal project at the moment and can't promise to look at it soon.

VoxValue commented 4 years ago

Hi @arthurdenner I rollback to the previous version and wait for your availability. Thanks in advance Fred

arthurdenner commented 4 years ago

I would say that is better if you try to contribute than wait for me :)

arthurdenner commented 4 years ago

@Fred2MTL, could you provide a reproducible repo with instructions regarding this? It's important for whoever is going to work on it.

VoxValue commented 4 years ago

@arthurdenner No problem. I will do that this week.

arthurdenner commented 4 years ago

@Fred2MTL, any updates?

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Antoine-O commented 4 years ago

I think I have the same problem. I am using Meteor with react/semantic-ui. I will check when the project is built if it changes something.

arthurdenner commented 4 years ago

I was able to reproduce this using Snowpack, but couldn't find a quick solution to the problem without evolving a rewrite of a lot of code since require is sync but import() is async.

Due to lack of time, I didn't put more work on it so any help and suggestion is appreciated.

Antoine-O commented 4 years ago

I don't know much on programming node package but could the package use project based json if not found in the package ?

wollo commented 3 years ago

I am facing the same issue when bundling the module via rollup. I think there are 3 potential solutions around:

  1. going back to the before 2.0 way of handling locales. That works for me well in rollup
  2. modifying the current version so that it handles both ways. So you export the locale folder as in version 1 and when somebody supplies an locale object instead of a locale string, it will use the object and not imports the json file
  3. you use dynamic import() with await instead of require in get locale. Bable (via plugin) Webpack, Rollup (via plugin) and Codesandbox are supporting this. So this might work

If you decide, what you prefer, I could try to contribute a patch

pzmosquito commented 3 years ago

Submitted PR https://github.com/arthurdenner/react-semantic-ui-datepickers/pull/664 to resolve this issue

arthurdenner commented 2 years ago

Hey everyone!

I know ESM support is a long-standing issue and there's no direct replace for require (a synchronous import) on ESM. So I've explored a few options given by some of you and if possible, I'd like your input and opinion on the options below.

Option 1 - Accept an object as the locale prop

The component can accept an object as it was in v1, never reaching the require() that breaks for ESM. Implementation: https://github.com/arthurdenner/react-semantic-ui-datepickers/commit/b75087912fe11d24378dbe44819656039db1633a

import ptBR from 'react-semantic-ui-datepickers/dist/locales/pt-BR.json';

<SemanticDatepicker locale={ptBR} />

A tradeoff is leaving to the consumer any switch of locales at runtime. Depending on the setup, one needs to either:

Option 2 - Bundle all locales with the datepicker

Similar to what was done in #664 but keeping the require for cjs builds. On the ESM build, all locales would be bundled and we set the state from an internal map. Implementation: https://github.com/arthurdenner/react-semantic-ui-datepickers/commit/ef135da9c4b0543cd1406971245e4d54a4a0fd98

<SemanticDatepicker locale="pt-BR" />

A tradeoff is that the bundle increases in size with every new locale and isn't tree-shakeable AFAIK. One may get a lot of unnecessary code - some extra kbs but still. A benefit is not worrying about switching locales at runtime.

@VoxValue @Antoine-O @wollo @pzmosquito @MutatedGamer I'm not sure how relevant this is for you anymore but would you mind sharing your opinion or proposing another solution?

pzmosquito commented 2 years ago

Thanks for putting this together. This is still relevant to me and my team. I have no problem going with either approach. Here's my 2 cents:

If you plan on refactoring the code properly to get rid of require in the future, go with option 2, because it will not require user code update. Otherwise go with option 1.

MutatedGamer commented 2 years ago

@arthurdenner may I propose following what react-datepicker does? https://github.com/Hacker0x01/react-datepicker#localization

Essentially, users of the dependency have to manually load the locale they want and "register" it. This registration saves the locale object in the browser to later be fetched by anything that needs to be formatted with locales in mind. Since the locale is really just passed down to date-fns, I assume the "en-US" locale is always loaded.


Another suggestion is to go with option 1, but have SemanticDatepicker import the "en-US" locale. This way the "default" locale is always loaded and can be used without error. If the consumer wants to use a non-default locale, it must load it first. I think this is a good option because it

  1. Is easy to implement (compared to the above)
  2. Prevents needing to load all locales
  3. Is a non-breaking change for anyone who currently uses the default locale
wollo commented 2 years ago

Yes, that is still relevant to me! I am still using the 1.x version in my applications and I would love to go to a newer 2.x version if possible. My praeference would be option 1 or option 1 with default locale en-US. But also option 2 would be OK.

pzmosquito commented 2 years ago

@arthurdenner any update?

gelonsoft commented 11 months ago

Quick fix - just create custom class override with required locale:

import SemanticDatepicker from "react-semantic-ui-datepickers";
import ruRU from 'react-semantic-ui-datepickers/dist/locales/ru-RU.json';

export class MyDatepicker extends SemanticDatepicker {
    get locale () {
        return ruRU
    }
}