fix(store): disable suppressing errors in selector

[splincode]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/ngxs/store/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

#843

What is the new behavior?

@NgModule({
 imports: [
   NgxsModule.forRoot([TasksState], {
       developmentMode: true,
       selectorOptions: { suppressErrors: false }
    })
 ]
})

Does this PR introduce a breaking change?

[ ] Yes
[x] No

[markwhitfeld]

Great idea. The internalSelectorOptions should not be exposed it was deliberately made internal. Maybe the internal selector options should inherit from the sharedSelectorOptions?

[splincode]

@markwhitfeld
What are the advantages of this interface internalSelectorOptions? We still need to use this opportunity anyway. Investigating this, I did not dare why we should use it as an internal parameter, if I would like to use it now:

@NgModule({
 imports: [
   NgxsModule.forRoot([TasksState], {
       developmentMode: true,
       selectorOptions: { injectContainerState: false }
    })
 ]
})

[markwhitfeld]

The injectContainerState setting will not be used as a permanent feature, but rather used internally as a transitionary step in upgrading to v4, so I don't want to add it to the public interface.
It will be used internally by a compat decorator/plugin.
For example a decorator that could be applied to a selector or class. For Example @UpgradeSelectorToV4(). This would be provided as part of a compat subpackage. I will be doing a PR for this soon.

[splincode]

@markwhitfeld

Do not you think that this way you complicate the transition? I think we need the whole team to discuss this moment! because the decorator will be much harder to support and why people will need to have extra dependencies in their code.

To upgrade to version 4, people can be asked to do so.

@NgModule({
 imports: [
   NgxsModule.forRoot([TasksState], {
       developmentMode: true,
       selectorOptions: { injectContainerState: false }
    })
 ]
})

If nothing works for them, it will be able for the 4th version to leave this injectContainerState flag to true as global settings.

[markwhitfeld]

I have always thought of the transition using the decorators which come from the compat library so that we can drop then after the transition, but keeping this as an option is growing on me...
What would you propose the syntax to be when specifying it at a state class level or for an individual selector?

[splincode]

@markwhitfeld

ngxs.config.ts

export const NGXS_CONFIG = {
     developmentMode: true,
     selectorOptions: { 
           // use global options
           injectContainerState: false
     }
};

app.module.ts

@NgModule({
 imports: [
   NgxsModule.forRoot([AppState], NGXS_CONFIG)
 ]
})
export class AppModule {}

app.state.ts

@State({
 name: 'appState',
 default: {
   ... 
 }
})
@SelectorsOptions({ 
     // use options for local any selector in state
     injectContainerState: true
 })
export class AppState {
   
   @Selector() 
   public static selectorA(state) {
     return state;
   }

   @Selector() 
   public static selectorB(state) {
     return state;
   }
 
   @Selector([]) 
   @SelectorOptions({ injectContainerState: true })
   public static selectorC(state) {
     return state;
   }

}

[markwhitfeld]

@sinedied what do you think of this proposal?

[markwhitfeld]

@splincode I think that we would need to rather have a @SelectorOptions(...) decorator to apply at the class level because you can define selectors outside of states.

[sinedied]

My opinion on this is that a global setting for the whole app would be enough, a configuration per state seems overkill to me.
The simple reason is that instead of an option, it's easier to have a distinct selector decorator with a different name for the transition time (that could be bundled in a compat package) if you need to keed the old behavior at specific places.

We already went this way even before the v4 option is available, by making our own OptimizedSelector decorator that behaves just like v4 will. Once v4 is release, a simple search/replace on our selector to put back the one from NGXS will be enough to make the migration 🎉

For people wanting to delay the migration, the easiest way for them to migrate would be one of this in my opinion:

• either set compatibility option to v3 globally and then selectively upgrade the selectors using a differently named one and move on once the full migration is done (like we plan to)
• set compatibility option to v4, and use a differently named selector to downgrade behavior to v3 (opposite of previous solution), can be a global search/replace at the beginning then progressing in the migration you replace the compat selector with the normal one. At the end, nothing left to do, just to remove the compat package

I think compatibility selectors to either upgrade or downgrade with a distinct name is simpler and better than an option, as it's easier to track in the app during the migration (and easier to implement for you).

[markwhitfeld]

Going ahead with this PR. We will create a general purpose @SelectorOptions which can be used to configure the options at class or method level.