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: ignore ./locale import from moment/min/moment-with-locales #533

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

ShGKme
Copy link
Collaborator

@ShGKme ShGKme commented Jan 16, 2024

Fixes compatibility between @nextcloud/webpack-vue-config and @nextcloud/moment@1.3.0 (since migration to Vite).

@nextcloud/moment uses moment/min/moment-with-locales.js, which 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'

See also:

@nextcloud/moment since v1.3.0 uses `moment/min/moment-with-locales.js`.
Which 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.

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme
Copy link
Collaborator Author

ShGKme commented Jan 16, 2024

A fix on @nextcloud/moment side could be better, cc @susnux if you have any ideas.

@susnux
Copy link
Contributor

susnux commented Jan 16, 2024

How would you fix it? The problem is that we otherwise need to import every local in @nextcloud/moment manually.

@ShGKme
Copy link
Collaborator Author

ShGKme commented Jan 16, 2024

How would you fix it? The problem is that we otherwise need to import every local in @nextcloud/moment manually.

I dunno, that is why I mentioned you =D

I didn't dig much into what happens in @nextcloud/moment and what/why was changed during the migration to Vite. The source issue is in the moment itself. That require is not correct in the moment's source, and it is so a classic issue that Webpack docs show the fix as an example of the IgnorePlugin usage. So we can fix it on the Webpack config side, but the fix on @nextcloud/moment seems less breaking.

Probably, import.glob in Vite + manual locales registration may work.

Anyway, I'm fine with merging this PR to fix the problem as well.

@susnux
Copy link
Contributor

susnux commented Jan 16, 2024

Probably, import.glob in Vite + manual locales registration may work.

This does only work with local imports not with modules 😶

@ShGKme ShGKme requested review from ChristophWurst and skjnldsv and removed request for skjnldsv January 24, 2024 13:13
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.

3 participants