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

fix: import locales and moment instead of moment-with-locales #803

Closed
wants to merge 1 commit into from

Conversation

ShGKme
Copy link

@ShGKme ShGKme commented Jan 22, 2024

moment/min/moment-with-locales.js works only in Node.js and is not compatible with Webpack bundling. It has an unused function localLocale that requires locales by invalid relative path ./locale. Though it is not used, Webpack tries to resolve it with require.context and fails.

Example of issue in Talk, same in Text and other apps:

WARNING in ./spreed/node_modules/moment/min/moment-with-locales.js 2159:16-50
Module not found: Error: Can't resolve './locale' in '<...>\talk-desktop\spreed\node_modules\moment\min'

The current solution via Vite plugin manually import all the locales from ES modules from dist and imports moment from the same dist with ESMs. Each module defines locale in this moment.

Alternative to:

See also:

@ShGKme ShGKme added bug Something isn't working 3. to review labels Jan 22, 2024
@ShGKme ShGKme self-assigned this Jan 22, 2024
lib/index.ts Outdated Show resolved Hide resolved
@ShGKme
Copy link
Author

ShGKme commented Jan 23, 2024

True, doesn't work in Forms..

@ShGKme ShGKme marked this pull request as draft January 23, 2024 16:40
@ShGKme ShGKme force-pushed the fix/import-moment-with-locales branch 2 times, most recently from 2c86fbb to e5d9674 Compare January 23, 2024 17:50
@ShGKme ShGKme requested a review from susnux January 23, 2024 17:51
@ShGKme ShGKme marked this pull request as ready for review January 23, 2024 17:51
@ShGKme ShGKme force-pushed the fix/import-moment-with-locales branch from e5d9674 to 79c12e9 Compare January 23, 2024 18:00
lib/moment-dist-moment.d.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No sure this looks much more complicated than the webpack changes?

@ShGKme
Copy link
Author

ShGKme commented Jan 23, 2024

It is more complicated than to fix on the @nextcloud/webpack-vue-config for sure. But it fixes a kind of regression.

I haven't found an easier solution, tried different moment dists, tried to play with disabling optimization. import.meta.glob doesn't work with packages.

The PR with a fix in the webpack config is ready for review as well, I'm fine with any way.

@nickvergessen nickvergessen removed their request for review January 24, 2024 08:48
@nickvergessen
Copy link

Sorry, not my level of code understanding :(

moment-with-locales doesn't work in Webpack due to invalid unused require

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme force-pushed the fix/import-moment-with-locales branch from 79c12e9 to 8e5f77a Compare January 24, 2024 09:12
@ShGKme
Copy link
Author

ShGKme commented Jan 24, 2024

Closing in flavor of nextcloud-libraries/webpack-vue-config#533

No issues now with @nextcloud/moment with Webpack using @nextcloud/webpack-vue-config

@ShGKme ShGKme closed this Jan 24, 2024
@susnux susnux deleted the fix/import-moment-with-locales branch January 25, 2024 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants