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

Typing inline generic operators seems much more annoying with 3.8 (coming from 3.6) #2161

Open
dmrickey opened this issue Apr 22, 2024 · 3 comments

Comments

@dmrickey
Copy link

dmrickey commented Apr 22, 2024

Let me know if I'm overcomplicating something. This is what we used to have in 3.6

const isTrailer = info.contentTypeId === ContentType.Trailer.id;
const insert = (item) => iif(isTrailer, insertItem(item), append([item]));
setState(
    patch({
        currentTranslations: insert(currentTranslation),
        defaultTranslations: insert(defaultTranslation),
        informationCollection: insert(info),
    })
);

for 3.8 this is what I've got to do now which seems needlessly verbose

const insert = <T extends SessionInformation | SessionTranslation>(item: T) =>
    iif(isTrailer, insertItem<T>(item as NoInfer<T>), append<T>([item] as NoInfer<T[]>));

Is there something I'm missing here or is it this complex now?

A couple more examples

const move = (collection: Array<SessionTranslation | SessionInformation>) => {
     const item = collection.find((x) => x.id === id);
     const index = collection.indexOf(item);
     return iif(index >= 0 && !!item, compose(removeItem(index), insertItem(item, 0)));
};
// to
const move = <T extends SessionInformation | SessionTranslation>(collection: Array<T>) => {
    const item = collection.find((x) => x.id === id);
    const index = collection.indexOf(item);
    return iif(index >= 0 && !!item, compose(removeItem<T>(index), insertItem<T>(item as NoInfer<T>, 0)));
};
const patchSeries = () => updateManyItems(() => true, patch({ isActive: content.isActive, isPublic: content.isPublic }));
setState(
    patch({
        showSeriesInformationCollection: patchSeries(),
    })
);
// to just doing it inline instead of having the self-documenting function name
setState(
    patch({
        showSeriesInformationCollection: updateManyItems(
            () => true,
            patch({ isActive: content.isActive, isPublic: content.isPublic })
        ),
    })
);
@markwhitfeld
Copy link
Member

markwhitfeld commented Apr 22, 2024

Hi @dmrickey
Thanks for the feedback. Yes, there were some fundamental changes in the typings for state operators. You can read about them in this PR, and a user actually wrote an article about the problem.
These changes were introduced to:

  • Provide better type safety: previously the operators would accept incorrect types as parameters because the type inference was working in the wrong direction. The changes involved shifting the type inference so that the return type of the operator could be used to infer the generic type based on the requirements of the call site.
  • Provide better intellisense: With the corrected inference direction, this allowed for the typescript linter to be able to provide correct and exact intellisense for the user of the operator.

This change placed a stricter constraint on how the operators are implemented, but better safety and ergonomics for the users of those operators. It is a very worth while tradeoff, because, typically, operators are only implemented in a few places, and used in many other places.

Just a comment on your types, it might be a bit easier if you use the NoInfer<T> on your function parameters. That would be the recommended place to put it to follow the new pattern.
For example: https://www.ngxs.io/snippets/operators#state-operator-code

PS. Typescript now actually added a built in NoInfer<T> utility type for solving this problem in TS 5.4.

@markwhitfeld
Copy link
Member

As an alternative (but a downgrade of experience in my opinion), you could enable your previous experience by wrapping each of the core operator functions with your own functions with the same name, and deliberately don't use the NoInfer<T> in your wrapper function's argument, but do the casting internally (similar to what you are doing above). You would then use these wrapped operators in place of the core ones.

This would degrade your type safety and intellisense enough to the point where the code is as permissive as it was before. Who needs those pesky types anyway! 😉

@dmrickey
Copy link
Author

Even with typing the input parameter I still have to cast the append call.

const insert = <T extends SessionInformation | SessionTranslation>(item: NoInfer<T>) =>
    iif(isTrailer, insertItem<T>(item), append<T>([item] as NoInfer<T[]>));

This is why so many people are moving away from typescript. const insert = (item) => iif(isTrailer, insertItem(item), append([item])); was far easier to read, but now it's just a huge jumble of types with NoInfer being mostly white noise because it's only needed because it's needed.

I'm not trying to be argumentative. I understand overall the tooling is better, but these short one-off examples are beyond ugly now and far less useful

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