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

Merge moment-duration-format plugin #3308

Closed
wants to merge 3 commits into from

Conversation

butterflyhug
Copy link
Contributor

@butterflyhug butterflyhug commented Jul 17, 2016

See #1048.

A quick pass at converting @jsmreese's plugin into the Moment module system and stripping out the obviously duplicated code vis-a-vis Moment's existing helper functions, etc. I also arbitrarily yanked out the "precision" config option, because I didn't want to keep the padZero function that was mostly a duplicate of Moment's zeroFill, and I didn't care to rework that bit of code right now.

This currently adds 4.8KB to the minified build, which is about +8% of the existing codebase. That seems a little heavy given our concerns about file size. On the other hand, there are a number of comments in the code, so this stat may be overstating the actual impact on file size.

I still need to port the tests before we merge this, but I probably won't bother to do so until we decide if the file size impact is prohibitive.

@jsmreese
Copy link

You'll want to take the code from the dev branch of moment-duration-format, not the master branch. A few issues are fixed there, and the default behaviour is changed to rounding, not truncating (with truncating as a configurable behaviour), which seems like a better default.

Also, the minified size of my plugin as it stands is ~1.8KB gzipped and ~4.4KB uncompressed. As you noted, the unminified code is full of comments -- those are stripped out by any minifier so won't affect the minified file size.

@maggiepint
Copy link
Member

What if we built this as a separate file? Seems like that should be possible, especially if we drop IE 8 support with moment 3. That would eliminate some of those utility imports.

@miloszsobczak
Copy link

what about this mr?

@ichernev
Copy link
Contributor

@butterflyhug so are there any reserved characters or anything so we could (in the future maybe) implement what we talked about in gitter : .format("[h:[m:]]s") to mean drop the pieces if the token is 0.

@butterflyhug
Copy link
Contributor Author

The plugin that I was porting in this PR is using [] to denote literal text, so we'd want to reserve something else if we want both behaviors. (Maybe {}?) That said -- I suspect we want to avoid a situation where we support both the "trim" feature from moment-duration-format and the bracket-based syntax that we were chatting about; mixing the two sounds bug-prone.

Assuming we prefer our syntax since it's a bit more flexible for developers, I'll try to take a crack at writing a version based on @jsmreese's dev branch that implements our syntax and removes the trim-related features. That's lower in my priority queue than the immutability stuff, though.

@ichernev
Copy link
Contributor

@butterflyhug I think ideally we should have both and maybe a flag to choose. Sometimes you want no trim so the existing code should support that.

I know its low priority, that is why I just want to reserve a token and maybe open an issue to see how many people care (with +1).

@maximou4391
Copy link

Hi all,
Is this PR going to be merged soon?
Thanks a lot, that would be awesome!
Have a good day.

type: ((type === 'escape' || type === 'general') ? null : type)

// calculate base value for all moment tokens
//baseValue: ((type === 'escape' || type === 'general') ? null : this.as(type))

Choose a reason for hiding this comment

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

nit: left over code?

@flyalex
Copy link

flyalex commented Sep 23, 2016

Hi!
Guys, will this feature come soon?
Thanks!

@georgeteo
Copy link

+1 on this PR please.

@butterflyhug
Copy link
Contributor Author

This remains on my todo list, but it's a lower priority for me than getting immutability ready for 3.0. When I come back to this, I'm likely to start over with porting @jsmreese's newer version of his original plugin instead of continuing with this particular porting effort.

Given these realities, I'm going to close this PR. If someone else wants to pick up the work before I get a chance to work on it again, please feel free.

marwahaha added a commit to marwahaha/ZZ-FORK-moment that referenced this pull request Nov 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants