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

Akita deep freeze is conflicting with moment.js date formatting logic (notably with mat-datepicker) #124

Closed
simeyla opened this issue Nov 22, 2018 · 8 comments · Fixed by #209

Comments

@simeyla
Copy link

simeyla commented Nov 22, 2018

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request
[ ] Other... Please describe:

Current behavior

When adding a moment.js date object to the store (in non production mode of course), the object gets frozen. This appears to be affecting the prototype (or some other global construct in moment), such that even a moment date object created completely outside of akita is broken when formatted.

I discovered this issue using a mat-datepicker with minimal default configuration and assigned a moment date value to it. Important: This date object hadn't even come from akita store, but I have other moment objects stored in the store.

In moment.js there is the following code which is part of the date formatting logic

    this._longDateFormat[key] = formatUpper.replace(/MMMM|MM|DD|dddd/g, function (val) {
        return val.slice(1);
    });

The above code is reached when formatting the moment with a lowercase format string (which is one of the values for defaultLongDateFormat such as L, LL, LLL). This occured for me by simply assigning a moment instance to a mat-datepicker which uses a lowercase l value by default. So in this instance the moment.js code above has key == "l" - and it then runs the regex on the format string for L and tries to assign it back onto _longDateFormat as a new key for future use.

The above line of code fails because Object.isFrozen(this._longDateFormat) == true and gives the extremely cryptic message ERROR TypeError: Cannot add property l, object is not extensible.

I just realized that possibly trying to format a date for l before I initialize akita may be the solution - but wanted to pass this on and document it the best I can because it was very obscure and others will surely run into this sooner or later.
Edit: Yes!, adding moment().format('l') before akita is initialized will fix this problem`, but would really love to hear your input.

Expected behavior

I would expect to be able to put a moment object in the store and it not to break moment for use elsewhere :-)

Minimal reproduction of the problem with instructions

I can come back to create a demo later if needed if the explanation above doesn't suffice.

What is the motivation / use case for changing the behavior?

Need to be able to put a moment date object in the store and it not break.

Environment


Angular version: X.Y.Z


Browser:
- [x] Chrome (desktop) version XX
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX
 
For Tooling issues:
- Node version: XX  
- Platform:  

Others:

  • Latest version of moment with latest angular.
  • I also updated akita (having seen that a recent change was made to the freezing logic). It still has this issue.

Thanks

@NetanelBasal
Copy link
Collaborator

I don't recommend to store moment object in the store. Why you're not saving the raw date and transform it in the service/component?

For example:
query.select(state => state.date).pipe(map(date => moment(date)))

@simeyla
Copy link
Author

simeyla commented Nov 23, 2018

Because I'm just putting the result of my API call into the store, which uses moment (I'm using ngswag typescript generator). Was hoping to just be able to use moment everywhere.

I'm not sure there's much you can do about it your end - but I wanted to make sure this was at least a known 'issue' the people could find if they get the same error. It's a super confusing error, and had I not known that akita 'freezes' objects I may have never figured it out!

Unless akita provided a way to intercept the 'freezing' I'm not sure what you could do, and at least for now just running the following code in my app initializer is sufficient to prevent this happening:

moment().format('l')

@NetanelBasal
Copy link
Collaborator

The freezing is a well-known technique in Redux, ngrx, ngxs, etc. It's to protect you from making mistakes and mutate your data.

Thanks for adding this issue here. If I find a solution to your specific problem, I will update the issue.

@DmitryEfimenko
Copy link
Contributor

here's related issue at the moment.js side.
Indeed it's a very obscure issue and it took me a while to figure out what's happening. Unfortunatelly, the solution suggested by @simeyla won't work for me. The API I work with (I don't have control over it) sends dates in all kinds of formats. Interestingly, that issue I referenced also mentions Akita (see last comment). I wonder if other stores do freezing in some other way that does not result in this issue.

@NetanelBasal
Copy link
Collaborator

@DmitryEfimenko You can disable the freezing protection if you're sure that no one will mutate the state.

@orangeswim
Copy link
Contributor

how do we disable freezing protection? I also ran into this issue when using @angular/material-moment-adapter with @angular/datepickermodule

@NetanelBasal
Copy link
Collaborator

Always run the prod mode.

orangeswim added a commit to orangeswim/akita that referenced this issue Apr 17, 2019
Adds deepFreezeFunction in StoreConfigOptions and unit tests for feature
Can use custom deepFreeze for complex objects in store

Resolves salesforce#124
@simeyla
Copy link
Author

simeyla commented Feb 12, 2021

Note: if you're using ImmerJS in conjunction with Akita and you were circumventing this issue by always using production mode, note that version 8 of ImmerJS will always freeze by default.

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 a pull request may close this issue.

4 participants