TypeScript: use two different types

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.

SantosGuillamot · 16 November 2020 10:03

This feature has been included in the latest release : Read the full Release Announcement .

luisherranz · 24 November 2020 08:08

This is a video of @orballo @david and I refactoring a complex theme @orballo is working on, with the new types released in the last version.

We introduced:

The new MergePackages util.
The new Data types.
Extending Data with new types, like FichasData.
Using the new type guards, like isPostType(data).
Creating new type guards, like isFichas(data).
The new Entity types.
Extending Entity, like FichasEntity.

Video:

I have to say, I am really happy with the result! I think the new types fix all the previous problems. Now everything makes sense and it is straightforward

@orballo please follow up once you have made the full refactoring. Let us know if you still have any problems!!

orballo · 18 December 2020 04:17

Hi guys!

I didn’t finish with the types we were working on yet, but while working on another project I run into a couple of things that I cannot figure out and maybe they are bugs. Please help me out here.

1. `Derived` with arguments is not callable

I’m trying to call a derived value that accepts an argument (Derived<Packages, number, string>) from inside another derived.

2. `Action` is not callable

I’m trying to call an action (Action<Packages>) from inside another action.

3. `Derived` that returns `string` cannot be used as `string`

I’m trying to pass a derived value that returns a string (Derived<Packages, string>) to parseFloat.

Here is the repo in case you need it https://github.com/orballo/invoice

luisherranz · 18 December 2020 08:37

I ran into the exact same problem yesterday

You don’t have a tsconfig.json file in your project. Add this one:

tsconfig.json

{
  "compilerOptions": {
    "target": "es2017",
    "moduleResolution": "node",
    "module": "commonjs",
    "allowSyntheticDefaultImports": true,
    "noEmit": true,
    "esModuleInterop": true,
    "resolveJsonModule": true,
    "jsx": "react",
    "allowJs": true
  }
}

I was also taking a look to see if we could ship a tsconfig.json file within the "frontity" package to make something like this work:

tsconfig.json

{
  "extends": "frontity"
}

That way if we make a change to our TypeScript configuration people don’t need to change their file.

It says in the docs that it uses “Node.js style resolution” so I guess it should work.

luisherranz · 18 December 2020 08:52

By the way, @david, I have a question for you.

Even if we use a data type guard like this:

// Get information about the current URL.
const data = state.source.get(state.router.link);
// Do nothing is this is not a post type.
if (!isPostType(data)) return null;
// Get the data of the post.
const post = state.source[data.type][data.id];

and data is PostTypeData after the if, post is any.

That is because data.type is string in PostTypeData and therefore TypeScript is not able to know the values.

If we use isPost or isPage, then it works:

// Get information about the current URL.
const data = state.source.get(state.router.link);
// Do nothing is this is not a post.
if (!isPost(data)) return null;
// Get the data of the post.
const post = state.source[data.type][data.id];

Now post is PostEntity. But that is not useful in case you have a single component for all the post types, like in mars:

<Switch>
  <Post when={isPostType(data)} />
</Switch>

I see you have also used a trick to manually cast the type in a component by passing data from the parent and typing the compnent, like this:

<Switch>
  <Post when={isPostType(data)} data={data} />
</Switch>

const Post: React.FC<{ data: PostTypeData }> = ({ data }) => {
  // ...
};

But that is not an ideal solution for all cases, in my opinion. It would be a bit like doing this:

// Get information about the current URL.
const data = state.source.get(state.router.link);
// Do nothing is this is not a post type.
if (!isPostType(data)) return null;
// Get the data of the post.
const post: PostTypeEntity = state.source[data.type][data.id];

So my question is… do we still need to add state.source.postTypes.xxx and state.source.taxonomies.xxx to solve this, or is there any other solution?

By the way, this would be the code in that case:

// Get information about the current URL.
const data = state.source.get(state.router.link);
// Do nothing is this is not a post type.
if (!isPostType(data)) return null;
// Get the data of the post.
const post = state.source.postTypes[data.type][data.id];

Also, right now I can’t figure out a way to get rid of the !isPostType(data) check. The only one would be to do a manual cast in state.source.get():

// Get information about the current URL.
const data = state.source.get<PostTypeData>(state.router.link);
// Get the data of the post.
const post = state.source.postTypes[data.type][data.id];

luisherranz · 18 December 2020 09:11

@orballo this is really cool by the way!!! https://invoice.orballo.dev/