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

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

Open
VoxValue opened this issue Feb 3, 2020 · 20 comments
Open

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

VoxValue opened this issue Feb 3, 2020 · 20 comments
Assignees
Labels
bug Something isn't working help wanted Extra attention is needed pinned

Comments

@VoxValue
Copy link

VoxValue commented Feb 3, 2020

❔ 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
Copy link
Owner

arthurdenner commented Feb 4, 2020

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. 😕

@arthurdenner arthurdenner added the question Further information is requested label Feb 4, 2020
@VoxValue
Copy link
Author

VoxValue commented Feb 8, 2020

@arthurdenner

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

  • it references a static path, starting from the '.' (aka current directory),
  • being included in all produced libraries (esm, gjs, etc.), it's not specific to the esm context.

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

arthurdenner commented Feb 9, 2020

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.

@arthurdenner arthurdenner added bug Something isn't working help wanted Extra attention is needed and removed question Further information is requested labels Feb 9, 2020
@arthurdenner arthurdenner pinned this issue Feb 9, 2020
@arthurdenner arthurdenner changed the title Existence of locale files Locales can't be found on ESM module and CodeSandbox Feb 9, 2020
@VoxValue
Copy link
Author

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

@arthurdenner
Copy link
Owner

arthurdenner commented Feb 10, 2020

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

@arthurdenner
Copy link
Owner

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

@VoxValue
Copy link
Author

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

@arthurdenner
Copy link
Owner

@Fred2MTL, any updates?

@stale
Copy link

stale bot commented May 24, 2020

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.

@stale stale bot added the stale label May 24, 2020
@arthurdenner arthurdenner added pinned and removed stale labels May 26, 2020
@arthurdenner arthurdenner self-assigned this Sep 5, 2020
@Antoine-O
Copy link

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

arthurdenner commented Sep 30, 2020

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

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

@wollo
Copy link

wollo commented Dec 22, 2020

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

Submitted PR #664 to resolve this issue

@arthurdenner
Copy link
Owner

arthurdenner commented Jan 17, 2022

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: b750879

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:

  • Import all locales needed to switch them
  • Dynamically import a new locale from the node_modules folder
  • Copy all locales to a public folder to dynamically import from it

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: ef135da

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

pzmosquito commented Jan 18, 2022

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

MutatedGamer commented Jan 21, 2022

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

wollo commented Jan 29, 2022

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

@arthurdenner any update?

@gelonsoft
Copy link

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
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed pinned
Projects
None yet
Development

No branches or pull requests

6 participants