ngxs / store

🚀 NGXS - State Management for Angular
http://ngxs.io
MIT License
3.52k stars 400 forks source link

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

Open dmrickey opened 3 months ago

dmrickey commented 3 months ago

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
showSeriesInformationCollection: updateManyItems(
    () => true,
    patch({ isActive: content.isActive, isPublic: content.isPublic })
),
markwhitfeld commented 3 months ago

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:

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 commented 3 months ago

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 commented 3 months ago

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