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

Update to NgRx 4 #17

Closed
sliekens opened this issue Jul 19, 2017 · 22 comments
Closed

Update to NgRx 4 #17

sliekens opened this issue Jul 19, 2017 · 22 comments

Comments

@sliekens
Copy link

It appears that ngrx-store-freeze is incompatible with NgRx 4.

@DorianGrey
Copy link

DorianGrey commented Jul 20, 2017

To be a bit more precise:
I've seen this issue as well while updating my template to ngrx4. It seems that parameters accessed by zone.js get freezed as well. At least that's what the browser console mentions:

TypeError: Cannot assign to read only property 'microTask' of object '[object Object]'
    at ZoneDelegate._updateTaskCount (polyfills.dll.js:5603)
    at Zone._updateTaskCount (polyfills.dll.js:5438)
    at Zone.runTask (polyfills.dll.js:5358)
    at drainMicroTaskQueue (polyfills.dll.js:5745)
    at <anonymous>

Uncaught TypeError: Cannot assign to read only property 'macroTask' of object '[object Object]'
    at ZoneDelegate._updateTaskCount (http://localhost:9987/polyfills.dll.js:5603:37)
    at Zone._updateTaskCount (http://localhost:9987/polyfills.dll.js:5438:34)
    at Zone.scheduleTask (http://localhost:9987/polyfills.dll.js:5397:22)
    at Zone.scheduleMacroTask (http://localhost:9987/polyfills.dll.js:5408:25)
    at http://localhost:9987/polyfills.dll.js:7009:33
    at proto.(anonymous function) (http://localhost:9987/polyfills.dll.js:6371:20)
    at XHRLocalObject.AbstractXHRObject (http://localhost:9987/polyfills.dll.js:4184:3)
    at new XHRLocalObject (http://localhost:9987/polyfills.dll.js:2208:13)
    at new InfoAjax (http://localhost:9987/polyfills.dll.js:4779:13)
    at Function.InfoReceiver._getReceiver (http://localhost:9987/polyfills.dll.js:11380:12)

Uncaught Error: macroTask 'setTimeout': can not transition to 'running', expecting state 'scheduled', was 'scheduling'.
    at ZoneTask._transitionTo (polyfills.dll.js:5682)
    at Zone.runTask (polyfills.dll.js:5331)
    at ZoneTask.invokeTask (polyfills.dll.js:5648)
    at ZoneTask.invoke (polyfills.dll.js:5637)
    at timer (polyfills.dll.js:6973)

Steps to reproduce:

@dojchek
Copy link

dojchek commented Jul 24, 2017

@DorianGrey
I'm getting the same exception.. All I do is simply provide 'storeFreeze' as meta-reducer:

StoreModule.forRoot(reducers, {
      metaReducers: [storeFreeze]
    }),

@DorianGrey
Copy link

DorianGrey commented Jul 24, 2017

Ok... I've debugged this a little bit, and it seems to have something to do with parameters added by @ngrx/router-store. These parameters may contain a router snapshot and additional linked data, and you may guess what happens when these get frozen.

Unfortunately, this means that not only some parameters of the state have to be skipped - even some dispatched actions have to be ignored completely.

I've hacked a solution for this, though I'm not that lucky with it:

import {Action, ActionReducer} from "@ngrx/store";

/**
 * A list of properties to ignore when deep-freezing.
 * Contains arguments that must not be modified in strict mode in general
 * (caller, callee, arguments) plus the `router` argument, which is added by @ngrx/router-store.
 */
const propBlackList: string[] = ["caller", "callee", "arguments", "router"];

/**
 * A regular expression to match `action.type` against.
 * Some actions have to be ignored in general, since freezing
 * any parts of them has undesirable side-effects.
 */
const ignoreActionsRegex: RegExp = /^(ROUTER?_|@ngrx)/;

function deepFreeze(o: any): void {
  Object.freeze(o);
  const hasOwnProp = Object.prototype.hasOwnProperty;
  Object
    .getOwnPropertyNames(o)
    .filter(p => propBlackList.indexOf(p) === -1)
    .forEach(prop => {
    if (hasOwnProp.call(o, prop)
      && o[prop] !== null
      && (typeof o[prop] === "object" || typeof o[prop] === "function")
      && !Object.isFrozen(o[prop])) {
      deepFreeze(o[prop]);
    }
  });

  return o;
}

export function storeFreeze(reducer: ActionReducer<any>): ActionReducer<any> {
  return function (state: any, action: Action) {
    if (ignoreActionsRegex.test(action.type)) {
      return reducer(state, action);
    } else {
      state && deepFreeze(state);
      // Ignore null/undefined values.
      if ((action as any).payload != null) {
        deepFreeze((action as any).payload);
      }
      const nextState = reducer(state, action);
      nextState && deepFreeze(nextState);
      return nextState;
    }
  };
}

Primary concern is configuration - the router field is the name of the routerReducer entry in my state definition, and thus may vary for other users. Already tried some things, but AoT did not like any of them...

Oh, and just for completeness: The above is a variant of this library and the underlying deep-freeze-strict. Had to modify both of them to make things work for now.

Just hope anyone has got a better idea than this hack, although I fear there are not that many solutions for this problem ...

@lekhnath
Copy link

lekhnath commented Aug 3, 2017

when will suport for ngrx v4 land?

@dojchek
Copy link

dojchek commented Aug 3, 2017

I think that we just have to wait for router-state serializer to be done.
ngrx/platform#188 (comment)

Once in, it should fix the following issue (which is pretty much the issue we are having here):
ngrx/platform#104

If you leave the router-store out of store, storeFreeze should work.

@LMFinney
Copy link

LMFinney commented Aug 7, 2017

Note: we're having the same problem, even though we're not using ngrx/router-store.

@strizzaflex
Copy link

There is a workaround that was merged into trunk of router-store. It's in the nightly build and should be in the next release.

ngrx/platform#188 - pull

@timbru31
Copy link

timbru31 commented Aug 17, 2017

Besides that, I'm receiving an UNMET PEER DEPENDENCY @ngrx/[email protected], because ngrx-store-freeze defines v2.2.1 of @ngrx/store

npm WARN [email protected] requires a peer of @ngrx/store@^2.2.1 but none was installed.

@dojchek
Copy link

dojchek commented Sep 11, 2017

Hey guys,
Any update on this one?

@aegyed91
Copy link
Collaborator

can somebody issue a pull request please?

i no longer keep playing with angular && the ecosystem. but i feel bad about these issues, cmon guys make a pull request 💃

also looking for maintainer because of the reaons mentioned above

@sliekens
Copy link
Author

@tsm91, perhaps you can start a fundraiser to resolve this issue? Make it worth your time.

@LMFinney
Copy link

@DorianGrey's suggestion makes sense as a workaround for an individual, but I don't think it should be pulled into the library, because it would secretishly disable freezing for any properties named "caller", "callee", "arguments", or "router". There's bound to be other programmers that would use those names at some point.

Isn't the core message here that the @ngrx router is mutating things that it shouldn't? Perhaps this metareducer is doing the right thing, and @ngrx itself is what needs to change.

I mentioned above that my team was seeing the same problem, and I'm pretty sure it's because we use wrapped kendo jquery widgets, and those widgets mutate objects that they receive. So, it was either our mistake in sending the original store objects that should have been mutated to kendo, or kendo's mistake for assuming that it could mutate what it received. Either way, it wasn't the fault of ngrx-store-freeze.

@DorianGrey
Copy link

because it would secretishly disable freezing for any properties named "caller", "callee", "arguments", or "router".

Just a note - I did not add the first three of that list for no reason: These properties exist on a function object and everything that derives from it. In any code that runs in strict mode, accessing these parameters is restricted, and may throw errors when attempting to freeze them. That is why these parameters are skipped in the deep-freeze library used by this project as well (missed the isFunction check in my hack, of course).

Isn't the core message here that the @ngrx router is mutating things that it shouldn't?

The library itself does not mutate parts of the provided state. The problems described above refer to an issue about storing objects (here: from zone.js) that have to be mutatable from outside, and thus cannot be frozen.

Perhaps this metareducer is doing the right thing, and @ngrx itself is what needs to change.

That already happend - the problem originially described here was cause by ngrx/router-store, which stores a router snapshot. However, that snapshot referenced a lot of stuff that cannot be frozen safely.
That's why the ability to provide a custom state serializer was introduced recently:
https://github.com/ngrx/platform/blob/master/docs/router-store/api.md#custom-router-state-serializer
By providing one, it is possible to only use a part of the snapshot, e.g. the route name and query parameters, thus skipping parameters that may not be frozen safely. The only "issue" that still remains is the warning about the mismatching peer dependency:
[email protected] requires a peer of @ngrx/store@^2.2.1 but none was installed.

So, it was either our mistake in sending the original store objects that should have been mutated to kendo, or kendo's mistake for assuming that it could mutate what it received.

The store content is intended to be immutable - it would not make sense to try to freeze the content explicitly if there is still code that attempts to mutate the objects it receives. So this sounds like a misuse in general, and not a problem of the store or the freeze meta reducer.

@brandonroberts
Copy link
Owner

Version 0.2.0 has been released with support for NgRx V4.x

@colinskow
Copy link

Well done for the great work @brandonroberts! Just one remaining issue: there is still a conflict with @ngrx/router-store. The router-store contains mutating objects. This needs to be excluded from the freeze, or router-store needs to be fixed to not contain mutating objects.

@zakhenry
Copy link

zakhenry commented Sep 25, 2017

@colinskow you should consider using a custom serializer with the router store as the router state has huge performance impact on ngrx due to the default serialized object. This was the cause of the issue with the devtools bogging down with performance. See https://github.com/ngrx/platform/blob/master/docs/router-store/api.md#custom-router-state-serializer

@colinskow
Copy link

@zakhenry thank you very much! It's now working perfectly. These instructions definitely need to be added to the docs because a lot of people are going to have this problem.

@zakhenry
Copy link

@colinskow no problem, docs updated in ngrx/platform#426

@brandonroberts
Copy link
Owner

I also added some docs in the README about @ngrx/router-store via #21

@markoffden
Copy link

Hmmm... for me issue is back after updating to NgRx 5.0, my ngrx-store-freeze version is 0.2.1

@duartealexf
Copy link

Issue still exists on ngrx 6.

@Frayion
Copy link

Frayion commented Sep 24, 2019

I am getting the same issue when using dispatch action, i cannot mutate property in my component to be pass inside dispatch.
ngrx 7 and ngrx-store-freeze 0.2.4

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

No branches or pull requests