Store: 🐞[BUG]: Incorrect Order of Interacting Selectors failure not obvious

[arturovt]

The order of selectors section is described in our docs.

[r-LaForge]

Thanks for the link, I have seen it.
~It doesn't mean it won't confuse, and doesn't mean it couldn't be handled with an error rather than seemingly undefined behaviour. I'm not sure what the implications of that would be though.~

I guess what I'm proposing is to handle it in such a way that the developer could become aware of an issue sooner. For example, throwing an error when the selectors are out-of-order rather than what seems to be undefined behaviour (the behaviour changed as I changed the order of the selectors). I'm not sure what the implications of that would be though.

[arturovt]

I think we can move that discussion to the v4 list of breaking changes issue https://github.com/ngxs/store/issues/827. Because currently that would really be a breaking change if we start throwing some errors.

[r-LaForge]

I would be for that

[markwhitfeld]

I wonder if we could potentially fix this order dependency or at least detect it and log something to the console in development mode?

[markwhitfeld]

@r-LaForge I have merged a PR that should fix this issue. Please could you test with the @ngxs/store@dev package and confirm if it is fixed for you or not.

[r-LaForge]

@markwhitfeld
That's awesome.
I will be able to try this weekend and let you know.

[Benno007]

Just a heads up, we upgraded from 3.5.1 to 3.6.1 yesterday and it caused a minor breakage in our code that worked in 3.5.1. We solved it by moving some code around (see below). I don't think the order of selectors as documented here necessarily applies to us, because we have completely separate state classes/selectors i.e. the selectors don't directly rely on each other - only in use in components for instance where we subscribe to one selector and then in the subscription, use a selector from another state class.

Looking at our problem/solution below, just wondering if it was expected that this change would break when using selectors as class properties?

Problem
In 3.5.1 we had this SearchService, where we declared a property of the class to call a selector in our UserState. doSearch was called by another component subscribing to a different state class (SearchState, which has no dependencies on our UserState)

@Injectable({
  providedIn: 'root'
})
export class SearchService {

  private readonly apiClient: ApiClient

// this line:
  private readonly currentCourtSystem$ = this.store.select(UserState.currentCourtSystem).pipe(filter(c => !!c))

  constructor (
    apiClientFactory: ApiClientFactory,
    private readonly store: Store
  ) {
    this.apiClient = apiClientFactory.build(APP_ROUTE_PATHS.search)
  }

  doSearch (
    query: string | undefined,
    searchType: SearchRequestType | undefined,
    pageNumber = 1,
    recordingId?: Uuid
  ): ApiResult<SearchResponse> {
    return this.currentCourtSystem$.pipe(
      switchMap(courtSystem =>
        this.apiClient.get<SearchResponse>({
          params: { ... snipped ...}
        })
      )
    )
  }
}

The selector for currentCourtSystem:

  @Selector() // state was undefined
  static currentCourtSystem (state: UserStateModel): CourtSystem | undefined {
    return state.courtSystem
  }

In a component, we dispatch GetSearchResultsAction which would then call doSearch. It was at this point, that we were getting 'cannot read property currentCourtSystem of undefined' (i.e. state was undefined in the above selector call).

Interestingly, we don't allow our app to render components until the 'currentCourtSystem' has been set into the store, so I can't see how it could have been a race condition.

combineLatest([
      this.store.select(SearchState.lastSearchTerm),
      this.store.select(SearchState.searchResultsScrollPosition).pipe(take(1)),
      this.store.select(SearchState.uniqueSearch),
    ]).pipe(
      filter(([lastSearchTerm]) => !!lastSearchTerm),
    ).subscribe(([lastSearchTerm]) => {
        this.store.dispatch(new GetSearchResultsAction(
          lastSearchTerm! as string,
          searchType as SearchRequestType,
          page as number,
          recordingId as Uuid)
        )
    })

Our solution
Simply moving the selector from a class property to in the doSearch method seemed to resolve the issue.

<+>UTF-8
===================================================================
--- src/app/core/header/search-bar/search.service.ts    (revision d2959f07d535ec1ac1fde17b0fe393c9f5e59c84)
+++ src/app/core/header/search-bar/search.service.ts    (date 1580253461622)
@@ -18,8 +18,6 @@

   private readonly apiClient: ApiClient

-  private readonly currentCourtSystem$ = this.store.select(UserState.currentCourtSystem).pipe(filter(c => !!c))
-
   constructor (
     apiClientFactory: ApiClientFactory,
     private readonly store: Store
@@ -41,7 +39,7 @@
-    return this.currentCourtSystem$.pipe(
+    return this.store.select(UserState.currentCourtSystem).pipe(filter(c => !!c)).pipe(
       switchMap(courtSystem =>
         this.apiClient.get<SearchResponse>({
           params: {

[splincode]

@markwhitfeld ping

[markwhitfeld]

@Benno007 Thank you so much for this in-depth bug report. This is definitely a regression. Would you mind _logging a new issue_ with the same description as above? I actually suspect that it is not related to the selector changes but potentially a change in 3.6 around the timing of the setup of the initial store state.

Although the code you included above is invaluable, would you mind creating a stack blitz reproduction for us. It would go a long way to providing a baseline to test against for this issue.