nextcloud-vue icon indicating copy to clipboard operation
nextcloud-vue copied to clipboard

Auto load locale from nextcloud for the Datepicker

Open skjnldsv opened this issue 6 years ago • 32 comments

Fix #137

skjnldsv avatar Dec 04 '18 14:12 skjnldsv

Will moment then be included twice if the app requires it as well? Or even be loaded three times by the server as well?

raimund-schluessler avatar Jan 10 '19 08:01 raimund-schluessler

@raimund-schluessler I don't get your question :thinking:

skjnldsv avatar Jan 10 '19 10:01 skjnldsv

The server ships moment.js in version 2.18.1 https://github.com/nextcloud/server/blob/master/package.json#L41 E.g. the contacts app requests moment.js in version 2.23.0 https://github.com/nextcloud/contacts/blob/master/package.json#L42 With this PR nextcloud-vue requires moment.js in version 2.22.2.

So my question is, will three different versions of moment.js be loaded when you open the contacts app? One from the server, and two with the contacts.js package (one directly, one in nextcloud-vue)?

Each moment.js version is at least 65 kB (gzip), more likely 225 kB (see https://bundlephobia.com/[email protected]). This sounds like quite some overhead.

raimund-schluessler avatar Jan 10 '19 10:01 raimund-schluessler

No, on contacts we use moment for the datepicker as well, so once this is merged, we will remove it from contacts :) Though this is a good question, how should we do that. How can we make sure we don't load it multipel times. For calendar (for example) they'll probably have to also load it for the whole app. If we have the same version as nextcloud-vue, can webpack merge the two of those?

On another note, with dynamic loading, we can load the locale separately from moment core, so it will be moment (48.9KB) + locale (avg 1.5KB). See #13

skjnldsv avatar Jan 10 '19 10:01 skjnldsv

Maybe this might help https://github.com/webpack/docs/wiki/optimization#deduplication

raimund-schluessler avatar Jan 10 '19 12:01 raimund-schluessler

Maybe this might help https://github.com/webpack/docs/wiki/optimization#deduplication

This option is redundant in webpack 2.

Seems perfect then!! Thanks @raimund-schluessler :hugs:

skjnldsv avatar Jan 10 '19 14:01 skjnldsv

Seems perfect then!! Thanks @raimund-schluessler hugs

This option is redundant in webpack 2.

well :man_shrugging:

ChristophWurst avatar Jan 11 '19 11:01 ChristophWurst

My 2c: for independent, isolated code like the Vue components or an app bundle it should be possible add async loading of locales for moment. But in Server we just assume that they are ready on page load. I don't think we can fix that.

For that webpack deduplication to work I assume the versions have to be identical. Webpack must not deduplicate otherwise.

ChristophWurst avatar Jan 11 '19 11:01 ChristophWurst

But in Server we just assume that they are ready on page load. I don't think we can fix that.

What do you mean by that? Couldn't we not load moment in our apps and use the server's version?

skjnldsv avatar Jan 11 '19 11:01 skjnldsv

Couldn't we not load moment in our apps and use the server's version?

We can. Tasks does not load its own moment version, just uses the server one.

raimund-schluessler avatar Jan 11 '19 11:01 raimund-schluessler

Sure you can use the server version, but this makes you dependent on server. And in the opposite direction updates of moment in server are more error prone as apps rely on it as well. This is the same issue with jQuery and the other dependencies.

Neither using the server version nor shipping a duplication with every app is ideal. But the latter at least offers isolation and easier maintenance.

ChristophWurst avatar Jan 11 '19 11:01 ChristophWurst

So the best would be to have moment integrated with nextcloud-vue, then?

skjnldsv avatar Jan 11 '19 12:01 skjnldsv

Yes. Deduplication from server is more important than the few kb. Also note that this PR is a bit old. So in the future contacts and the vue-components will probably ship the same moment version so it will be merged.

rullzer avatar Jan 11 '19 12:01 rullzer

Any work on this? Date picker's localisation would be useful.

dartcafe avatar Mar 13 '19 21:03 dartcafe

@dartcafe none so far. We should try to see what we can really do on this ! :thinking:

skjnldsv avatar Mar 14 '19 08:03 skjnldsv

@ChristophWurst could we have locale and such in nextcloud-server for this one?

skjnldsv avatar Apr 15 '19 11:04 skjnldsv

@ChristophWurst could we have locale and such in nextcloud-server for this one?

http://blog.wuc.me/nextcloud-server/modules/l10n.html#getlocale?

ChristophWurst avatar Apr 15 '19 11:04 ChristophWurst

No, sorry, I meant window.monthNamesShort, window.dayNamesShort & window.firstDay

skjnldsv avatar Apr 15 '19 11:04 skjnldsv

What's the status here? :)

georgehrke avatar Oct 09 '19 13:10 georgehrke

What's the status here? :)

no idea, @ChristophWurst would have liked a better way of loading the locales from server, but there is still no packages like @nextcloud/lang or something!

I guess we should maybe get this in? What do you think @ChristophWurst ?

Also, it means we will now ship moment and will require the app that uses our lib to implement the dynamic loading themselves

skjnldsv avatar Oct 09 '19 13:10 skjnldsv

no idea, @ChristophWurst would have liked a better way of loading the locales from server, but there is still no packages like @nextcloud/lang or something!

Well, there is @nextcloud/l10n, which provides getLocale() and getLanguage()

georgehrke avatar Oct 09 '19 13:10 georgehrke

Well, there is @nextcloud/l10n, which provides getLocale() and getLanguage()

My bad, it was about window.firstDay, window.monthNamesShort and window.dayNamesShort :)

skjnldsv avatar Oct 09 '19 13:10 skjnldsv

My bad, it was about window.firstDay, window.monthNamesShort and window.dayNamesShort

I guess we could include a getFirstDay(), getMonthNamesShort(), etc. in @nextcloud/l10n?

@ChristophWurst

georgehrke avatar Oct 09 '19 13:10 georgehrke

https://github.com/nextcloud/nextcloud-l10n/pull/30

georgehrke avatar Oct 09 '19 14:10 georgehrke

nextcloud/nextcloud-l10n#30 got merged. So we now have:

  • getFirstDay()
  • getDayNames()
  • getDayNamesShort()
  • getDayNamesMin()
  • getMonthNames()
  • getMonthNamesShort()

georgehrke avatar Oct 09 '19 15:10 georgehrke

@georgehrke mind to take over this one? :)

skjnldsv avatar Oct 09 '19 15:10 skjnldsv

Should we include the async loading of locales here or rather right away in @Nextcloud/moment?

georgehrke avatar Oct 09 '19 15:10 georgehrke

@georgehrke even if you include it here, you will need your own implementatin of the dynamic moment loading on your webpack config :)

skjnldsv avatar Oct 09 '19 15:10 skjnldsv

@georgehrke even if you include it here, you will need your own implementatin of the dynamic moment loading on your webpack config :)

Because of https://github.com/nextcloud/nextcloud-vue/pull/146#discussion_r333006574?

ChristophWurst avatar Oct 22 '19 15:10 ChristophWurst

Closing since https://github.com/nextcloud/nextcloud-vue/pull/2397 replaced it.

raimund-schluessler avatar Dec 16 '21 14:12 raimund-schluessler