TypeScript: use two different types

luisherranz · 30 April 2020 05:34

Description

Right now we are merging the types of a package with the types needed for actions and components. It’d be great to to merge somehow the packages types.

For example, imagine this package:

import { Package } from "frontity/types";

interface MyPackage extends Package {
  state: {
    myPackage: {
      someState: string;
    };
  };
  actions: {
    myPackage: {
      someAction: Action<MyPackage>;
    };
  };
}

export default MyPackage;

If any of your actions or components need state.router.link then you need to add it to MyPackage as an optional:

import { Package } from "frontity/types";
import Router from "@frontity/router/types";

interface MyPackage extends Package {
  state: {
    router?: Router["state"]["router"]; // <- here
    myPackage: {
      someState: string;
    };
  };
  actions: {
    myPackage: {
      someAction: Action<MyPackage>;
    };
  };
}

export default MyPackage;

But these manual merges are:

Ugly. You have to do them manually.
They get verbose if you need to add multiple things like state, actions and libraries.
Unnecessary for the MyPackage type when you use it to type your package in src/index.js. I’d say they are even bad because you have optional types that are not really optional.
Unnecessary when you import this type in other packages. Because this types from other packages doesn’t belong to it. They are only there because they are used internally in some action or component.

Possible solution

Initial proposed solution (click to open)

I propose to use two different types in the types.ts file, one exclusively for the package, and the other a merge with the external packages for actions and components.

import { Package } from "frontity/types";
import Router from "@frontity/router/types";

interface MyPackage extends Package {
  state: {
    myPackage: {
      someState: string;
    };
  };
  actions: {
    myPackage: {
      someAction: Action<Packages>;
    };
  };
}

export default MyPackage;

export type Packages = Router & MyPackage;

I think it’s a cleaner approach, although it means people need to understand this a bit better because they need to use these to types in different places.

They have to remember to use Packages in their Action and Derived.

someAction: Action<Packages>;
someDerived: Derived<Packages>;

They have to remember to import MyPackage in the src/index.ts file:

import MyPackage from "../types";

const myPackage: MyPackage = {
  state: {
    myPackage: {
      someState: ”some state”
    }
  },
  actions: {
    myPackage: {
      someAction: ({ state }) => {
        // Can access state.router.link.
      }
    }
  }
}

export default myPackage;

They have to remember to use Packages for their React components.

import { Connect } from "frontity/types";
import { Packages } from "../../types";

const Comp: React.FC<Connect<Packages>> = ({ state }) => {
  // Can access state.router.link.
};

Same for useConnect hook:

import { useConnect } from "frontity";
import { Packages } from "../../types";

const Comp: React.FC = () => {
  const { state } = useConnect<Packages>();
  // Can access state.router.link.
};

External packages will keep using the default import of types:

import MyPackage from "my-package/types";

But this time it will only contain the types of MyPackage and nothing else.

Maybe instead of Packages we can call it Types or just Frontity (because it is the whole Frontity for this package).

@david, is it possible to do the merge of the types with a Merge helper like this:

import { Package, Merge } from "frontity/types";
import Router from "@frontity/router/types";
import Source from "@frontity/source/types";

export type Packages = Merge<Router, Source MyPackage>;

Any thoughts? Feedback?

Implementation Proposal

Final Implementation

luisherranz · 15 May 2020 13:22

luisherranz · 15 May 2020 13:23

We can also try to find a good TypeScript merge tool, from libraries like https://github.com/pirix-gh/ts-toolbelt.

David · 25 May 2020 18:16

I think it totally makes sense! Also, changing the types this way shouldn’t affect to people using TypeScript at this moment.

The only thing is that we should show some examples in our docs to make clear why it should work like this.

I would prefer Packages, it’s the most meaningful name for me.

luisherranz:

@david, is it possible to do the merge of the types with a Merge helper like this:

import { Package, Merge } from "frontity/types";
import Router from "@frontity/router/types";
import Source from "@frontity/source/types";

export type Types = Merge<Router, Source MyPackage>;

It is, although I would look for an external library as you proposed. Let me see what I find.

luisherranz · 26 May 2020 09:59

I have updated the OP to reflect that.

Awesome! Let’s try to do the switch then

I want to do a deep look at the current Types on https://github.com/frontity/frontity/pull/415 so that may also be a good opportunity to do the switch.

Let me know what you find!

David · 27 May 2020 13:52

I did a quick test in our frontity.org repository and it looks like we can simply use the intersection operator to do a deep merge of all the packages.

Also, if some package depends on other packages (as Router and Source do) you can still pass all the merged packages to them in the same type definition.

export type Packages = FrontityOrg &
  Router<Packages> &
  Source<Packages> &
  Html2React &
  Analytics;

luisherranz · 28 May 2020 06:55

Perfect, thanks @david.

Let’s do it then

Do you all agree to use Packages? @mmczaplinski what do you think?

orballo · 28 May 2020 08:05

Packages sounds good to me

mmczaplinski · 28 May 2020 18:19

Yeah, Packages sounds fine to me

David · 9 June 2020 10:42

Well, I tried to implement types following what we discussed here for the analytics packages and some problems appeared. In order to avoid them we can follow these rules:

Do not use generics for exported code that is mean to be imported by other packages
Avoid exporting types that are only used internaly (the [key: string] part)
When an action could be reimplemented, try to pass the less possible amount of packages to that action type so it’s more difficult to have a type conflict.

PS: I wanted to write here yesterday what I found but I finally wrote a long comment in the GitHub PR. You can check that here: https://github.com/frontity/frontity/pull/471#issuecomment-641201225

luisherranz · 12 June 2020 08:11

Thanks @David. The explanation of https://github.com/frontity/frontity/pull/471#issuecomment-641201225 is great

I guess that what we were missing in https://www.youtube.com/watch?v=WKTXO9ziZ6g was that we were importing the actions (code) from the original @frontity/analytics package.

I’ll try to review the PR today.

luisherranz · 17 August 2020 07:54

We’ve been using this approach lately with success, but we just bumped into a problem.

Right now our types have a name field, which is defined as the final string, like this:

interface SomePackage extends Package {
  name: "@some-org/some-package";
  // Other types, like state, actions...
}

interface OtherPackage extends Package {
  name: "@some-org/other-package";
  // Other types, like state, actions...
}

When we merge more than one type containing a hardcoded name, TS 3.9 returns never so it’s not usable.

export type Packages = SomePackage & OtherPackage; // Returns `never`.

This used to work on TS 3.8.

The possible solutions are:

We get rid of name in the types. I think we’re not using it and we have that information elsewhere, but I have to take a look to confirm this.
Use string for the name type, like this:

interface SomePackage extends Package {
  name: string;
  // Other types, like state, actions...
}

luisherranz · 17 August 2020 08:04

Confirmed. We’re not even using it to extract the package names for the state.frontity.package array, we’re using the names from the frontity.settings.js file: https://github.com/frontity/frontity/blob/dev/packages/core/src/server/utils/initial-state.ts#L20

Then I guess we can get rid of it. I’ll do a PR.

luisherranz · 17 August 2020 10:49

I just remember the reason I used the hardcoded package names back then. It was to do the match of the types in the frontity.settings.ts file and have some type safety in that file.

Something like this. The state inside the ExtensionExample1 and ExtensionExample2 is based on a DeepPartial of the state of those types. The matching is done thanks to the name field.

import { Settings } from "frontity/types";
import ExtensionExample1 from "@frontity/extension-example-1/types";
import ExtensionExample2 from "@frontity/extension-example-2/types";

const settings: Settings<ExtensionExample1 | ExtensionExample2> = {
  name: "site-1",
  state: {
    frontity: {
      url: "https://test.frontity.io",
    },
  },
  packages: [
    "@frontity/tiny-router",
    "@frontity/wp-source",
    {
      name: "@frontity/extension-example-1",
      state: {
        comments: {
          prop2: 2,
        },
      },
    },
    {
      name: "@frontity/extension-example-2",
      state: {
        theme: {
          prop1: 1,
        },
      },
    },
  ],
};

module.exports = settings;

I’ll think what we can do. If this is worth the trouble or not.

luisherranz · 25 August 2020 17:34

I’ve done a video to explain how a frontity.settings.ts file works:

Link to Loom

It looks like we are going to need a MergePackages util to solve this problem:

import { MergePackages } from "frontity/types";

export type Packages = MergePackages<SomePackage, OtherPackage>;

mmczaplinski · 24 September 2020 00:27

I’ve played around with the implementation and came up with the following type for merging the packages. Works great, the only thing that I don’t like is that the Packages have to be passed as a tuple Merge<[Package1, Package2]> instead of individual parameters like Merge<Package1, Package>:

// Change the type of the package `name` from the string literal
// like "@frontity/wp-source" to just `string`.
type NameToString<T> = 'name' extends keyof T 
  ? Omit<T, 'name'> & { name: string }
  : T;

type MergePackages<T extends unknown[], U = T[number] > = 
  (U extends any ? (k: NameToString<U>)=> void : never) extends ((k: infer I)=> void) ? I : never


// ---------- Example

type Package1 = { name: 'a', x: string, y: string}
type Package2 = { name: 'b', z: string}

const packages1: MergePackages<[Package1, Package2]> = { name: 'test', x: 'hello', y: 'hi', z: 'hola' }

You can check it out in the TS playground:

luisherranz · 24 September 2020 09:44

Thanks Michal! I’ll add it to the https://github.com/frontity/frontity/tree/package-name branch and investigate if I can make it work with args instead of tuples

luisherranz · 2 October 2020 07:31

I would do something like this:

type MergePackages<
  P1 extends Package,
  P2 extends Package,
  P3 extends Package = {},
  P4 extends Package = {},
  P5 extends Package = {},
  P6 extends Package = {},
  P7 extends Package = {},
  P8 extends Package = {},
  P9 extends Package = {},
  P10 extends Package = {}
> = Omit<P1, "name"> &
  Omit<P2, "name"> &
  Omit<P3, "name"> &
  Omit<P4, "name"> &
  Omit<P5, "name"> &
  Omit<P6, "name"> &
  Omit<P7, "name"> &
  Omit<P8, "name"> &
  Omit<P9, "name"> &
  Omit<P10, "name">;

type Packages = MergePackages<Package1, Package2, Package3>;

I know it’s really ugly compared to Michal’s solution, but I think that tuples is less standard in terms of syntax 🤷

Also, I think we can remove name instead of replacing it for a string because it’s not required for that use. And if someone wants to extend several packages at once that were merged using MergePackages he/she can add it back.

interface MyPackage extends MergePackages<Package1, Package2> {
  name: "my-package";
  // ...
}

mmczaplinski · 2 October 2020 18:36

yeah, that’s fine because it’s an implementation detail Actually, I think it’s a better solution than my typescript jiu jitsu

David · 6 November 2020 13:18

Final implementation

New types

Added the MergePackages utility. It simply removes the name property from each package and returns the intersection of all of them, as specified in the implementation proposal. It is exported from frontity/types.

import { MergePackages, Derived } from "frontity/types";
import Source from "@frontity/source/types";
import Router from "@frontity/router/types";

interface MyPackage {
  state: {
    // Derived prop that depends on source and router packages.
    someProp: Derived<Packages>;
  };
}

// All packages merged together.
type Packages = MergePackages<Source, Router, MyPackage>;

Moved the frontity namespace from Package to the new Frontity interface. Now, if you need to access properties like state.frontity.url, state.frontity.rendering, etc. you have to import Frontity from frontity/types and include it in MergePackages as any other package.

import { Frontity, MergePackages, Derived } from "frontity/types";

interface MyPackage {
  state: {
    // Derived prop that depends on the frontity namespace.
    someProp: Derived<Packages>;
  };
}

// All packages merged together.
type Packages = MergePackages<Frontity, MyPackage>;

Other changes

Adapted all packages to use MergePackages and Frontity types if needed.
Refactored the Settings type to improve package types resolution.