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

Add a strip function to remove non-critical moment properties #1455

Closed
ichernev opened this issue Feb 1, 2014 · 8 comments
Closed

Add a strip function to remove non-critical moment properties #1455

ichernev opened this issue Feb 1, 2014 · 8 comments
Milestone

Comments

@ichernev
Copy link
Contributor

ichernev commented Feb 1, 2014

We've been adding properties to the moment object here and there and now most of them are used only at the parsing stage, revealing information about what exactly got parsed and how, but most users don't care and if you happen to have a big array of objects this also leads to higher memory footprint.

So lets add a function moment.fn.strip(), that would remove all unnecessary properties, and keep only: _isAMomentObject, _isUTC, _l, _d. _offset.

@icambron
Copy link
Member

Instead of strip, I think it should just be an option at creation time. "lean" or somesuch. My thinking is:

  • We should keep the mutable surface area as small as possible.
  • Creating and then removing it means it has to wait for the GC to come get it

Also worth noting: the expensiveness of parsingFlags only comes up when you actually parse a lot strings; cloning just copies the reference (shallow clone, which is totally right).

@timrwood
Copy link
Member

Another use case to keep in mind, moment-timezone uses the _a and _tzm properties to check if it an offset was included in the input string.

https://github.com/moment/moment-timezone/blob/9fc647a10b9efa5566b46442e0b5b6abe5d8eeb9/moment-timezone.js#L285

@icambron
Copy link
Member

Ah, that's a good point. It also puts a big hole in my idea in #1754 to just make clone explicitly copy the fields (which might be a good optimization for clone whether or not we do immutability); we don't know what fields defined in plugins need to be copied.

@ichernev
Copy link
Contributor Author

In the latest version there is a way for plugins to add the fields they're adding to moment to the whitelist. Latest moment-timezone is doing that. Just edit moment.momentProperties.

The thing is I broke moment-timezone for a bit and nobody noticed. I wonder how many other plugins are broken. We can copy fields beginning with underscore for now and warn for each field separately -- hopefully more people would see / report those back to us, and then we'll pinpoint the offensive plugin.

@icambron
Copy link
Member

@ichernev Oh cool, that gives me a path back to my optimization idea. I can just remove items from momentProperties (i.e. before there are plugins, it's just empty), assign them explicitly, and then only iterate through the momentProperties if there's anything in it.

Re: breaking plugins: we can just look through the ones on the docs and see if they fiddle with the moment object directly. I suspect most don't, since they're mostly about building, wrapping, or formatting moments, not altering their behavior like TZ does.

@ichernev
Copy link
Contributor Author

I didn't quite get the optimization thing.

@icambron
Copy link
Member

Sorry, referencing other tickets. I'll explain with code and some perf tests.

@maggiepint
Copy link
Member

So, two years later now, I think there is quite a bit of code that relies on parsing flags and other variables being the way they are, and this change doesn't make sense. As such, I'm going to close this for the sake of cleaning house on issues. I'll assign a v3 milestone in case we want to find it down the road when considering total rewrite style scenarios.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants