search

ngrx / platform

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

Return type of selectQueryParam is incorrect #4286

Closed Tezra closed 1 month ago

Tezra commented 3 months ago

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

router-store

Minimal reproduction of the bug/regression with instructions

Sorry I can't provide a minimal example, but for some reason my StackBlitz example always returns undefined (https://stackblitz.com/edit/angular-63sasz?file=src%2Fapp%2Fmy-counter%2Fmy-counter.component.ts, incase you can fix the configuration). This bug is a discrepancy between the static and runtime typing though. (example snippet below).

The selector from selectQueryParam says the return type is string, however the actual type at runtime is string[] | string | undefined, based on if the query param appears 0 (undefined), 1 (string) or more (string[]) times.

import { Component } from '@angular/core';
import { RouterReducerState } from '@ngrx/router-store';
import { Store, select, createFeatureSelector } from '@ngrx/store';
import { Observable } from 'rxjs';
import * as RouteStore from '@ngrx/router-store';
import { tap } from 'rxjs/operators';

export const {
  selectCurrentRoute, // select the current route
  selectQueryParams, // select the current route query params
  selectQueryParam, // factory function to select a query param
  selectRouteParams, // select the current route params
  selectRouteParam, // factory function to select a route param
  selectRouteData, // select the current route data
  selectUrl, // select the current url
} = RouteStore.getRouterSelectors(
  createFeatureSelector<RouterReducerState>('router')
);
const aSelector = selectQueryParam('a');
const bSelector = selectQueryParam('b');

@Component({
  selector: 'app-my-counter',
  templateUrl: './my-counter.component.html',
  styleUrls: ['./my-counter.component.css'],
})
export class MyCounterComponent {
  a$: Observable<string>; // Actually Observable<string[] | string | undefined>
  b$: Observable<string>;

  constructor(private store: Store<any>) {
    this.a$ = this.store.pipe(select(aSelector), tap(console.log));
    this.b$ = this.store.pipe(select(bSelector), tap(console.log));
  }
}

Expected behavior

For the runtime type to match the Typescript type.

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

As far as I can tell, all.

Other information

Fixing this would be a breaking change, but the incorrect type is an issue for strict projects trying to take advantage of the string[] behavior, and if the url is modified in an unexpected way, may crash pages that think they are type safe.

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

timdeschryver commented 3 months ago

Hi, it seems like you're right! Do you want to create a PR for this? We can schedule this for v18 version and document this as a breaking change.

Tezra commented 3 months ago

I can. I don't think it will take that long to fix but I probably won't be able to work on it till this weekend.

timdeschryver commented 1 month ago

@Tezra we're currently preparing our v18 release, do you think you can create a PR for this?

Tezra commented 1 month ago

@timdeschryver The codespace container is failing to build for me (error below). So seems I can't create a PR at the moment or run the tests.

As far as I can tell, the only change needed for the PR is in @ngrx/router-store/src/models.d.ts; line 7 selectQueryParam: (param: string) => MemoizedSelector<V, string | undefined>; -> selectQueryParam: (param: string) => MemoizedSelector<V, string | string[] | undefined>;

(Plus updating any typing elsewhere that clashes with this.)

=================================================================================
2024-05-27 20:00:13.343Z: Running blocking commands...
2024-05-27 20:00:13.393Z: $ devcontainer up --id-label Type=codespaces --workspace-folder /var/lib/docker/codespacemount/workspace/platform --mount type=bind,source=/.codespaces/agent/mount/cache,target=/vscode --user-data-folder /var/lib/docker/codespacemount/.persistedshare --container-data-folder .vscode-remote/data/Machine --container-system-data-folder /var/vscode-remote --log-level trace --log-format json --update-remote-user-uid-default never --mount-workspace-git-root false --omit-config-remote-env-from-metadata --skip-non-blocking-commands --expect-existing-container --config "/var/lib/docker/codespacemount/workspace/platform/.devcontainer/devcontainer.json" --override-config /root/.codespaces/shared/merged_devcontainer.json --default-user-env-probe loginInteractiveShell --container-session-data-folder /workspaces/.codespaces/.persistedshare/devcontainers-cli/cache --secrets-file /root/.codespaces/shared/user-secrets-envs.json
2024-05-27 20:00:13.637Z: @devcontainers/cli 0.56.1. Node.js v18.20.2. linux 6.5.0-1021-azure x64.
2024-05-27 20:00:14.004Z: Running the onCreateCommand from devcontainer.json...

2024-05-27 20:00:14.009Z: sudo cp .devcontainer/welcome-message.txt /usr/local/etc/vscode-dev-containers/first-run-notice.txt
2024-05-27 20:00:14.199Z: Running the postCreateCommand from devcontainer.json...

2024-05-27 20:00:14.207Z: yarn install
2024-05-27 20:00:14.401Z: 2024-05-27 20:00:14.409Z: 2024-05-27 20:00:14.412Z: yarn install v1.22.19
2024-05-27 20:00:14.543Z: [1/5] Validating package.json...
2024-05-27 20:00:14.553Z: 2024-05-27 20:00:14.563Z: error @********@18.0.0-beta.1: The engine "node" is incompatible with this module. Expected version "^18.19.1 || ^20.11.1 || >=22.0.0". Got "18.16.0"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
2024-05-27 20:00:14.677Z: {"outcome":"error","message":"Command failed: /bin/sh -c yarn install","description":"The postCreateCommand in the devcontainer.json failed.","containerId":"1911f4d440635ff8be6aecc97617f60e6daa5dd2ed990dbcdc00db62029d50ab"}
2024-05-27 20:00:14.684Z: postCreateCommand failed with exit code 1. Skipping any further user-provided commands.

2024-05-27 20:00:14.690Z: Error: Command failed: /bin/sh -c yarn install
2024-05-27 20:00:14.697Z:     at wY (/.codespaces/agent/bin/node_modules/@devcontainers/cli/dist/spec-node/devContainersSpecCLI.js:235:130)
2024-05-27 20:00:14.698Z:     at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
2024-05-27 20:00:14.702Z:     at async nl (/.codespaces/agent/bin/node_modules/@devcontainers/cli/dist/spec-node/devContainersSpecCLI.js:227:4393)
2024-05-27 20:00:14.706Z:     at async tl (/.codespaces/agent/bin/node_modules/@devcontainers/cli/dist/spec-node/devContainersSpecCLI.js:227:3738)
2024-05-27 20:00:14.715Z:     at async rl (/.codespaces/agent/bin/node_modules/@devcontainers/cli/dist/spec-node/devContainersSpecCLI.js:227:2942)
2024-05-27 20:00:14.725Z:     at async fs (/.codespaces/agent/bin/node_modules/@devcontainers/cli/dist/spec-node/devContainersSpecCLI.js:227:2386)
2024-05-27 20:00:14.732Z:     at async q$ (/.codespaces/agent/bin/node_modules/@devcontainers/cli/dist/spec-node/devContainersSpecCLI.js:464:1488)
2024-05-27 20:00:14.738Z:     at async iK (/.codespaces/agent/bin/node_modules/@devcontainers/cli/dist/spec-node/devContainersSpecCLI.js:464:960)
2024-05-27 20:00:14.747Z:     at async gAA (/.codespaces/agent/bin/node_modules/@devcontainers/cli/dist/spec-node/devContainersSpecCLI.js:481:3660)
2024-05-27 20:00:14.755Z:     at async BC (/.codespaces/agent/bin/node_modules/@devcontainers/cli/dist/spec-node/devContainersSpecCLI.js:481:4775)
2024-05-27 20:00:14.760Z: devcontainer process exited with exit code 1

====================================== ERROR ====================================
2024-05-27 20:00:14.764Z: Failed to create container.
=================================================================================
2024-05-27 20:00:14.768Z: Error: Command failed: /bin/sh -c yarn install
2024-05-27 20:00:14.772Z: Error code: 1302 (UnifiedContainersErrorFatalCreatingContainer)