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

[bugfix] toJSON: Fix isValid so that toJSON works after a moment is frozen #3383

Closed
wants to merge 1 commit into from

Conversation

ben-ng
Copy link

@ben-ng ben-ng commented Aug 19, 2016

A small fix so that moments can be serialized with JSON.stringify after Object.freeze has been used on it.

@maggiepint
Copy link
Member

We can't use this in Moment v2 because it technically supports IE 8. We have some options here:

  • Bump this to v3, but v3 will be immutable anyways, so maybe it doesn't matter
  • just skip it
  • Shim the object.isfrozen call to return false if the function isn't available

Preferences?

@ichernev
Copy link
Contributor

@maggiepint I think this particular change can be slightly rewritten to support both IE8 an above. If there is such thing as frozen -- check if it is, otherwise assume it isn't (because it can't be).

@ben-ng just check if isFrozen exists first, and it will do the trick.

@mattjohnsonpint
Copy link
Contributor

@ichernev - I'm using "Needs Revision" rather than "todo" as it's more descriptive.

@ben-ng please add the check as @ichernev suggested. Thanks.

@ichernev
Copy link
Contributor

ichernev commented Sep 3, 2016

Merged in de9799b

@ichernev ichernev changed the title Fix isValid so that toJSON works after a moment is frozen [bugfix] toJSON: Fix isValid so that toJSON works after a moment is frozen Sep 3, 2016
@ichernev ichernev closed this Sep 3, 2016
ichernev added a commit that referenced this pull request Sep 3, 2016
[bugfix] toJSON: Fix isValid so that toJSON works after a moment is frozen
@ben-ng
Copy link
Author

ben-ng commented Sep 6, 2016

Thanks for making the fix and merging this while I was on vacation!

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

Successfully merging this pull request may close these issues.

4 participants