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

Moment #13

Closed
ChristophWurst opened this issue Sep 4, 2018 · 17 comments · Fixed by #4219
Closed

Moment #13

ChristophWurst opened this issue Sep 4, 2018 · 17 comments · Fixed by #4219
Labels
1. to develop Accepted and waiting to be taken care of component Component discussion and/or suggestion

Comments

@ChristophWurst
Copy link
Contributor

ChristophWurst commented Sep 4, 2018

As discussed at #9 (comment), I'd like to see a component for the moment.js integration, a.k.a. x seconds/minutes/hours ago thingy.

The existing Vue components suggested didn't work as expected, esp. in regards to l10n. Moreover, they wouldn't work with our live timestamp update magic (cc @nickvergessen). Hence, I've created my own component. This is what it looks like:

<template>
	<span class="live-relative-timestamp"
		  :data-timestamp="timestamp"
		  :title=title>{{ formatted }}</span>
</template>

<script>
	import moment from 'moment';

	if (typeof OC !== 'undefined') {
		moment.locale(OC.getLocale());
	}

	export default {
		name: "Moment",
		props: [
			'timestamp',
			'format'
		],
		computed: {
			title () {
				return moment.unix(this.timestamp / 1000).format(this.format || 'LLL');
			},
			formatted () {
				return moment.unix(this.timestamp / 1000).fromNow();
			}
		}
	}
</script>

It works well and can be easily integrated. The live updating also seems to work with this.

Caveats

  • Pulls in moment.js (obviously) but also lots of translations. Webpack combines some stuff, but it's still rather big. We basically have the same problem in server, but we'd now load the locals twice.
  • Depends on a global var. I've added a check for it's existence, though.
@ChristophWurst ChristophWurst added the 2. developing Work in progress label Sep 4, 2018
@ChristophWurst ChristophWurst changed the title Moment.js integration Com: Moment Sep 4, 2018
@ChristophWurst ChristophWurst changed the title Com: Moment Comp: Moment Sep 4, 2018
@rullzer
Copy link
Contributor

rullzer commented Sep 4, 2018

How big is big? As we cache JS stuff pretty agressively.

@ChristophWurst
Copy link
Contributor Author

How big is big? As we cache JS stuff pretty agressively.

~1/4 MB according to https://github.com/jmblog/how-to-optimize-momentjs-with-webpack#measurements.

@skjnldsv
Copy link
Contributor

skjnldsv commented Sep 4, 2018

1/4 like 0.25 or 1-4MB? ^^

@ChristophWurst
Copy link
Contributor Author

1/4 like 0.25

250kB :D

@skjnldsv
Copy link
Contributor

skjnldsv commented Sep 4, 2018

For a component that only display a relative date, this is crazy ^^"
If we only display by units like x sec ago x days ago x hours ago, shouldn't this be universal across languages?

@ChristophWurst
Copy link
Contributor Author

For a component that only display a relative date, this is crazy ^^"

Yes, kind of. But the main lib is tiny. It's just the translations that make it a huge bundle of small packages that, apparently, sum up a lot.

If we only display by units like x sec ago x days ago x hours ago, shouldn't this be universal across languages?

I doubt it. We would have to provide our own translations.

So I'd simply go with the slighly oversized moment.js now and we can always replace the underlying library if we find a replacement. Users of the component wouldn't and shouldn't notice.

@skjnldsv
Copy link
Contributor

skjnldsv commented Sep 4, 2018

We could use the t() function like we do everywhere?

@ChristophWurst
Copy link
Contributor Author

We could use the t() function like we do everywhere?

But then we'd have to translate all moment strings ourselves. If we go with the existing package, it already has all the translation strings.

@ChristophWurst
Copy link
Contributor Author

Again, let's start with something simple (existing, big momentjs thingy) and we can always replace that by our own code. To make progress, I'd like to keep things as simple as possible now.

@skjnldsv
Copy link
Contributor

skjnldsv commented Sep 4, 2018

Is there that many translations needed? I mean, it's like any other translation son our website?

@nickvergessen
Copy link
Contributor

the thing is, this will be included like 10 times in the end?
activity, notifications, announcements, talk, ...

Is there that many translations needed? I mean, it's like any other translation son our website?

Well Days in long and short (Mon. Monday), same for months (dec. december), etc.

@nickvergessen
Copy link
Contributor

But yeah im fine with a huge bump at the moment and fixing it properly afterwards

@skjnldsv
Copy link
Contributor

skjnldsv commented Sep 4, 2018

Looking at the moment lib, I'm pretty sure we can ship this with a dynamic load of the locale. So moment (48.9KB) + locale (avg 1.5KB) we should be fine.
But we need a proper webpack dynamic loading setup :)

@skjnldsv
Copy link
Contributor

skjnldsv commented Sep 5, 2018

Small references: alternative lib that does the same and is as supported: https://github.com/date-fns/date-fns/
More features and explanations: date-fns/date-fns#275 (comment)

And webpack reference for moment: https://webpack.js.org/plugins/ignore-plugin/#ignore-moment-locales

@skjnldsv skjnldsv added the component Component discussion and/or suggestion label Sep 7, 2018
@skjnldsv skjnldsv changed the title Comp: Moment Moment Sep 7, 2018
@skjnldsv
Copy link
Contributor

skjnldsv commented Sep 7, 2018

Small update, I successfully implemented moment into contacts.
With dynamic loading! :)
It works very nicely! I'll share the config soon :)

@skjnldsv
Copy link
Contributor

skjnldsv commented Sep 7, 2018

On you entry file:

// CSP config for webpack dynamic chunk loading
// eslint-disable-next-line
__webpack_nonce__ = btoa(OC.requestToken)

// Correct the root of the app for chunk loading
// OC.linkTo matches the apps folders
// OC.generateUrl ensure the index.php (or not)
// eslint-disable-next-line
__webpack_public_path__ = OC.generateUrl(OC.linkTo('contacts', 'js/'))

Webpack config:

	output: {
		[...]
		chunkFilename: 'chunks/[name].js'
	},
	[...]
	plugins: [
		[...]
		new webpack.IgnorePlugin(/^\.\/locale$/, /moment$/)
	],

Babel config

{
	[...]
	"plugins": ["@babel/plugin-syntax-dynamic-import"]
}

Then just import

	import('moment/locale/' + this.locale)
		.then(e => {
			// success
		})
		.catch(e => {
			// failure
		})

@skjnldsv
Copy link
Contributor

skjnldsv commented Oct 1, 2018

Correction:

// We do not want the index.php since we're loading files
__webpack_public_path__ = OC.linkTo('contacts', 'js/')

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop Accepted and waiting to be taken care of component Component discussion and/or suggestion
Projects
None yet
4 participants