ngrx / platform

Reactive State for Angular
https://ngrx.io
Other
7.96k stars 1.95k forks source link

@ngrx/store: Public API is incomplete #3811

Open RobertDiebels opened 1 year ago

RobertDiebels commented 1 year ago

Which @ngrx/* package(s) are the source of the bug?

store

Minimal reproduction of the bug/regression with instructions

Current Behavior Currently the NGRX store Public API is not exposing types that are part of the public API. In effect the public API is incomplete and this hinders customizing/extending NGRX functionality tremendously.

Reproduction Compilation error due to incorrect type-definition:

import { createActionGroup } from "@ngrx/store";

export function extendedCreateActionGroup<Source extends string>(sourceName: Source) {
  return createActionGroup({
    source: sourceName, // <- Will cause a compilation error
    events: {},
  });
}

Expected but, compilation error due to missing type-definition StringLiteralCheck:

import { createActionGroup } from "@ngrx/store";

export function extendedCreateActionGroup<Source extends string & StringLiteralCheck<Source, 'source'>>(sourceName: Source, entityNameSingular: string) {
  return createActionGroup({
    source: sourceName, 
    events: {},
  });
}

Minimal reproduction of the bug/regression with instructions

Expected Behavior

The NGRX public API should expose all types that are used within it's public API. The types that are internal only should not be exposed.

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s)

NgRx: At the very least every version released with createActionGroup() up until 15.4.0

Other information

No response

I would be willing to submit a PR to fix this issue

brandonroberts commented 1 year ago

@RobertDiebels we expose types to the public API as necessary, in the case that we want to change them in the future. In most cases, these can be resolved without exposing internal types. We'll take a look and see if this is a reasonable change

RobertDiebels commented 1 year ago

@brandonroberts Awesome! Thanks for the reply. When you reach a positive decision I'd be most willing to write up a PR. Do you have a possible ETA on the decision? I'll try to make some time to work on a PR in case you do. Either way, thanks for considering. Would help me out a lot.

RobertDiebels commented 1 year ago

@brandonroberts @markostanimirovic Could you provide me with some information about the status of this ticket? As since https://github.com/ngrx/platform/pull/3887 several types are no longer exposed and this breaks a large chunk of our code-base. As I cannot explicitly return an ActionGroup as of 16.0.1 because I cannot import the type.

collettemathieu commented 11 months ago

Alternatively, you can use a string literal type to replace the Source type. For example, you can do:

import { createActionGroup } from "@ngrx/store";

export function extendedCreateActionGroup(sourceName:`_${string}`) {
  return createActionGroup({
    source: sourceName,
    events: {},
  });
}
RobertDiebels commented 11 months ago

Alternatively, you can use a string literal type to replace the Source type. For example, you can do:

@collettemathieu Thank you for the response. I used the Source-type as a simple example to illustrate that extending/composing functionality offered by NGRX whilst ensuring type-saftey is effectively made impossible. Unless you manually copy-paste the "public" types.

My current assumption is that NGRX is moving in the opposite direction of this request/bug-report. Since more types have been removed from the public APIs since 16.0.1.

Not exposing types and changing them at a later time has the same net-effect as exposing them and changing them. That being, compiler warnings. However, the former cannot ensure type-safety when users extend functionality and as a result imposes a detrimental developer experience. Whilst the latter does offer this and results in a better developer experience.

Again, I would love to contribute a PR towards achieving this but, my guess is that this ticket will not get traction and I will move to a different client if/when it does, removing immediate relevancy for myself and the client.

ducthang-vu commented 10 months ago

Alternatively, you can use a string literal type to replace the Source type. For example, you can do:

@collettemathieu Thank you for the response. I used the Source-type as a simple example to illustrate that extending/composing functionality offered by NGRX whilst ensuring type-saftey is effectively made impossible. Unless you manually copy-paste the "public" types.

My current assumption is that NGRX is moving in the opposite direction of this request/bug-report. Since more types have been removed from the public APIs since 16.0.1.

Not exposing types and changing them at a later time has the same net-effect as exposing them and changing them. That being, compiler warnings. However, the former cannot ensure type-safety when users extend functionality and as a result imposes a detrimental developer experience. Whilst the latter does offer this and results in a better developer experience.

Again, I would love to contribute a PR towards achieving this but, my guess is that this ticket will not get traction and I will move to a different client if/when it does, removing immediate relevancy for myself and the client.

Strongly agree.

markostanimirovic commented 10 months ago

My current assumption is that NGRX is moving in the opposite direction of this request/bug-report. Since more types have been removed from the public APIs since 16.0.1.

Not exposing types and changing them at a later time has the same net-effect as exposing them and changing them. That being, compiler warnings. However, the former cannot ensure type-safety when users extend functionality and as a result imposes a detrimental developer experience. Whilst the latter does offer this and results in a better developer experience.

It's important to mention that public API has not changed in any minor/patch version, because these types were not part of the public API. You probably used deep imports and that's the reason why it worked before.

Anyway, this feature request will be considered soon.

RobertDiebels commented 10 months ago

@markostanimirovic I had to double check this. The public API, i.e. type exports/declarations, has changed between 15.4.0 and 16.0.1. In the sense that the declaration files action_group_creator_models.d.ts and feature_creator_models.d.ts were removed.

These removed declaration files exported the types: ActionName, ActionGroupConfig, ActionGroup, FeatureSelector and NestedSelectors. These type declarations have now been moved to action_group_creator.d.ts and feature_creator.d.ts and are no longer exported. Effectively making them private types instead of public types hence, breaking the public API.

I realize that typescript uses type-inference and from that point of view the public API has not changed. However, from another point of view the declarations were exported and removed which did break the public API.

I can agree that we probably should have depended on the declarations provided by index.d.ts. On the other hand, I would state that if type declarations weren't supposed to be public then they shouldn't have been exported in the x_models.d.ts files. Since it is expected behavior for declaration files that exported types are always exposed outside that particular declaration file and the entire package.

wgi-mchlep commented 4 months ago

Not exporting the types also forces projects relying on @typescript-eslint/typedef to either change linting, create separate specific linting setups or just plain eslint-disable (next line/file).

Also if they were meant to be private why are they used as return types and not exported?

@typescript-eslint/typedef