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

🐞[BUG]: v18.1.5 / v18.1.6 silently break some state actions without error or logs #2286

Open
janpapenbrock opened this issue Dec 23, 2024 · 2 comments

Comments

@janpapenbrock
Copy link

Thank you all for your continued efforts into this package and keeping it up to speed with recent Angular releases and signals, much appreciated!

Happy holidays.

Affected Package

@ngxs/store >= 18.1.5 (tested 18.1.5 and 18.1.6).

Is this a regression?

Yes, @ngxs/store@18.1.4 works fine.

Description

Some of our state actions simply no longer patch the state.

I assume this is intended with the changes of #2231.

However, this is unexpected for us, and leaves us a bit scared of breaking actions which might not be covered by our automated test layers, breaking our app in random places.

We could not see any errors or other log messages, but might not make use of all of ngxs' logging capabilities. Is there a way to be notified when these null-ops are enabled for certain actions so that we can e.g. send it to our logging tool to find all of our affected actions?

🔬 Minimal Reproduction

Pretty specific and probably we're doing it wrong. I suspect the code change in ngxs/store itself makes sense, I rather see an issue with transparency / documentation here.

I do realize that mergeMap is more appropriate than tap here in the first place.

I also understand that you prefer a full reproduction example, and I can provide one if necessary to clarify the issue. But as the cause is pretty clear I saved myself the initial effort.

// Worked until 18.1.4:

  @Action(Initialize)
  initialize(ctx: StateContext<TM>, action: Initialize): Observable<any> {
    // other stuff happening here
    
    return of(true).pipe(
      tap(() => this.getAggregations(ctx)),
      // ^^ ------------------------------ this tap will become an issue from 18.1.5
      tap(() => {
        // need to call via action for proper cancellation behavior
        return this.store.dispatch(new AnotherAction(action.token));
      }),
      // ^^ ------------------------------ this tap will become an issue from 18.1.5
      mergeMap(() => this._afterInitialization(ctx)),
      map(() => true),
    );
  }

Working with mergeMap instead of tap:

// Working from 18.1.5:

  @Action(Initialize)
  initialize(ctx: StateContext<TM>, action: Initialize): Observable<any> {
    // other stuff happening here
    
    return of(true).pipe(
      mergeMap(() => this.getAggregations(ctx)),
      // ^^ ------------------------------ mergeMap instead of tap
      mergeMap(() => {
        // need to call via action for proper cancellation behavior
        return this.store.dispatch(new AnotherAction(action.token));
      }),
      // ^^ ------------------------------ mergeMap instead of tap
      mergeMap(() => this._afterInitialization(ctx)),
      map(() => true),
    );
  }

🔥 Exception or Error

patchState called inside getAggregations() does not have an effect anymore, but also does not report an error.

Environment


Libs:
- @angular/core version: 18.2.13
- @ngxs/store version: 18.1.5 / 18.1.6
@arturovt
Copy link
Member

Could you provide a minimal reproducible example?

@janpapenbrock
Copy link
Author

Just wanted to give an update here on the progress we're making in resolving this issue in our app.

As stated before, I believe this is not a issue with the fix that has been made in itself. For me the issue is rather a "breaking change hidden in a patch release" sort of thing.

In our app, so far we have identified seven actions (which equals 1-2% of our actions) which have been broken by this, which have been working for a long time before.

I'll give tangible examples of the silly things we did in our actions to have them be affected, once I've gotten around to find and fix them all.


For now, though, here's how we're currently tracking the issue to Sentry, so that we get to know when noops happen during runtime:

import { captureException } from '@sentry/angular';

function noop(...args) {
  const ex = new Error('State noop encountered; payload keys: ' + JSON.stringify(args.map(arg => Object.keys(arg))));
  captureException(ex);
  console.error(ex);
}

We're applying that in a postinstall hook in package.json, as in this Gist: https://gist.github.com/janpapenbrock/66c1d1344910ba40b86bbe19b51f36af

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

2 participants