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

Typescript declaration size #1425

Closed
1 of 2 tasks
skokenes opened this issue Nov 18, 2019 · 19 comments
Closed
1 of 2 tasks

Typescript declaration size #1425

skokenes opened this issue Nov 18, 2019 · 19 comments

Comments

@skokenes
Copy link

Question

  • I've checked documentation and searched for existing issues
  • I tried the spectrum channel

I have a fairly complex set of models that form a big store. This store is shared between a few services, so I have the store defined in its own package that my other services import from.

I am trying to generate typescript declarations for the models in this store so that my other services that use this store or sub models can get the correct typings. I am not sure if I am doing something wrong but I am getting a declaration file that is 7.4mb and about 109K LOC. This is causing poor performance in my IDE as I make changes.

Looking at the declaration file, I can see that the resulting size is due to reproducing the same type definitions over and over wherever they are used in my model. For example, I have a model type called Expression that is used by many other models in my store. This definition gets a unique declaration in every model that is used, rather than reusing a single type declaration for Expression.

My questions are:

  1. Is this expected behavior?
  2. Is there a way to avoid this so that I get slim, faster compiled declarations?
  3. Any other tips for how to architect or produce declarations for this type of project?

Thanks

@mweststrate
Copy link
Member

mweststrate commented Nov 19, 2019 via email

@k-g-a
Copy link
Member

k-g-a commented Nov 19, 2019

Make sure to specify concrete return types for views/actions which return other nodes, otherwise inferred return types are really huge.

@skokenes
Copy link
Author

Those solutions don't seem to address my problem unfortunately. See the following example repo:
https://github.com/skokenes/mst-typescript-dec-test

In src, I create a store called ModelA with a simple string. Then I create two stores that wrap that store:

  • ModelB: this uses ModelA as a nested property the way one typically does in MST, with inferred typing
  • ModelC: this uses ModelA as a nested property but with a hardcoded typing that references ModelA's type

When I build the declarations file for these 3 models, ModelB creates a completely new type for the nested ModelA. ModelC does not, but manually hardcoding every type this way with MST is pretty tough.

Is there a reason that ModelB creates new types for nested complex types instead of reusing the existing typings? Can this be avoided?

In a small model like this it doesn't really matter. But if you are building a complex store composed of many smaller stores, the declarations file quickly blows up as it reproduces store types over and over. For example, I am building a data analytics tool that uses MST. It has a complex store made up of smaller models, one of which is called Formula. As you can imagine, formulas are used in many places in an analytics tool. Currently, I have it referenced in 5 other models, which are in turn composed into larger models that feed the root store. When I create the declarations file, the typing for Formula is redefined 383 times. This is starting to grind down my IDE performance.

@Bnaya
Copy link
Member

Bnaya commented Nov 20, 2019

See my PR,
https://github.com/skokenes/mst-typescript-dec-test/pull/1/files
Using that pattern is verbose, but better than hand-writing specific interfaces

const ModelAInferred = t.model({
  foo: t.string
});


type ModelAFactoryType = typeof ModelAInferred;
interface ModelAFactoryInterface extends ModelAFactoryType {}
export const ModelA: ModelAFactoryInterface = ModelAInferred;
export interface IModelAStore extends Instance<ModelAFactoryInterface> {}

const ModelBInferred = t.model({
  a: ModelA
});

type ModelBFactoryType = typeof ModelBInferred;
interface ModelBFactoryInterface extends ModelBFactoryType {}
export const ModelB: ModelBFactoryInterface = ModelBInferred;
export interface IModelBStore extends Instance<ModelBFactoryInterface> {}

const ModelCInferred = t.model({
  // using that will create another declaration!
  // b: ModelBInferred
  b: ModelB
});
type ModelCFactoryType = typeof ModelCInferred;
interface ModelCFactoryInterface extends ModelCFactoryType {}
export const ModelC: ModelCFactoryInterface = ModelCInferred; 
export interface IModelCStore extends Instance<ModelCFactoryInterface> {}

@Bnaya
Copy link
Member

Bnaya commented Nov 20, 2019

Output types, only one IModelType is emitted per created model factory

import { IModelType, Instance, _NotCustomized, ISimpleType } from "mobx-state-tree";
declare const ModelAInferred: IModelType<{
    foo: ISimpleType<string>;
}, {}, _NotCustomized, _NotCustomized>;
declare type ModelAFactoryType = typeof ModelAInferred;
interface ModelAFactoryInterface extends ModelAFactoryType {
}
export declare const ModelA: ModelAFactoryInterface;
export interface IModelAStore extends Instance<ModelAFactoryInterface> {
}
declare const ModelBInferred: IModelType<{
    a: ModelAFactoryInterface;
}, {}, _NotCustomized, _NotCustomized>;
declare type ModelBFactoryType = typeof ModelBInferred;
interface ModelBFactoryInterface extends ModelBFactoryType {
}
export declare const ModelB: ModelBFactoryInterface;
export interface IModelBStore extends Instance<ModelBFactoryInterface> {
}
declare const ModelCInferred: IModelType<{
    b: ModelBFactoryInterface;
}, {}, _NotCustomized, _NotCustomized>;
declare type ModelCFactoryType = typeof ModelCInferred;
interface ModelCFactoryInterface extends ModelCFactoryType {
}
export declare const ModelC: ModelCFactoryInterface;
export interface IModelCStore extends Instance<ModelCFactoryInterface> {
}
export {};

@skokenes
Copy link
Author

Awesome! Yes its a pain to have to write this out, but its way better than manually typing everything.

Just curious, can you explain how it works? I don't totally follow why this behaves differently.

@Bnaya
Copy link
Member

Bnaya commented Nov 20, 2019

I got to these kind of pattens a while ago when nested models killed my IDE.
I've noticed that when you hover a variable with interface type, you see the interface name,
But if you hover variable with type from type declaration, you see the whole structure.
So my assumption is that typescript interface create some kind of "named cache point",
that is good for tsserver performance(=editing experience) and declaration size

A related typescript issue is:
microsoft/TypeScript#25023

@skokenes
Copy link
Author

skokenes commented Nov 20, 2019

I see, thanks.

How are you handling union types? Those are still recreating some props for me, though not necessarily blowing things up for me completely since they at least are reusing nested complex types.

Take the following input for example:

type ForDirectExtend<T> = T;

const _ModelA = t.model({
  foo: t.string
});

interface ModelAFactoryInterface extends ForDirectExtend<typeof _ModelA> {}
export const ModelA: ModelAFactoryInterface = _ModelA;

const _ModelB = t.model({
  bar: t.number
});

interface ModelBFactoryInterface extends ForDirectExtend<typeof _ModelB> {}
export const ModelB: ModelBFactoryInterface = _ModelB;

export const ModelC = t.model({
  a: ModelA
});

const _ModelD = t.union(ModelA, ModelB, ModelC);
interface ModelDFactoryInterface extends ForDirectExtend<typeof _ModelD> {}
export const ModelD: ModelDFactoryInterface = _ModelD;

This produces the following declaration file:

import { IModelType, _NotCustomized, ModelCreationType, ITypeUnion, ModelInstanceTypeProps, ModelSnapshotType, ISimpleType } from "mobx-state-tree";
declare type ForDirectExtend<T> = T;
declare const _ModelA: IModelType<{
    foo: ISimpleType<string>;
}, {}, _NotCustomized, _NotCustomized>;
interface ModelAFactoryInterface extends ForDirectExtend<typeof _ModelA> {
}
export declare const ModelA: ModelAFactoryInterface;
declare const _ModelB: IModelType<{
    bar: ISimpleType<number>;
}, {}, _NotCustomized, _NotCustomized>;
interface ModelBFactoryInterface extends ForDirectExtend<typeof _ModelB> {
}
export declare const ModelB: ModelBFactoryInterface;
export declare const ModelC: IModelType<{
    a: ModelAFactoryInterface;
}, {}, _NotCustomized, _NotCustomized>;
declare const _ModelD: ITypeUnion<ModelCreationType<import("mobx-state-tree/dist/internal").ExtractCFromProps<{
    foo: ISimpleType<string>;
}>> | ModelCreationType<import("mobx-state-tree/dist/internal").ExtractCFromProps<{
    bar: ISimpleType<number>;
}>> | ModelCreationType<import("mobx-state-tree/dist/internal").ExtractCFromProps<{
    a: ModelAFactoryInterface;
}>>, ModelSnapshotType<{
    foo: ISimpleType<string>;
}> | ModelSnapshotType<{
    bar: ISimpleType<number>;
}> | ModelSnapshotType<{
    a: ModelAFactoryInterface;
}>, ModelInstanceTypeProps<{
    foo: ISimpleType<string>;
}> | ModelInstanceTypeProps<{
    bar: ISimpleType<number>;
}> | ModelInstanceTypeProps<{
    a: ModelAFactoryInterface;
}>>;
interface ModelDFactoryInterface extends ForDirectExtend<typeof _ModelD> {
}
export declare const ModelD: ModelDFactoryInterface;
export {};

@Bnaya
Copy link
Member

Bnaya commented Nov 20, 2019

union inferring internal stuff regarding the model factory, what leads to that behaviour.
I believe that if the type that passed to the types.model() would be "named typed" and not inferred it would solve most of the problem.
I've tried to find a cleaver way to do so, haven't succeed yet.
types.model has some limitation with passing interfaced value.
Maybe we can create a different variant of types.model that will work with it.
Screen Shot 2019-11-21 at 0 58 19

@skokenes
Copy link
Author

Yea I see what you mean. Another problem I am having is that I am exporting these models to be used by other libraries, but my models include flows which don't work for me when compiling to JS and importing into another library. I get a "Action requires parent context" error for any action called involving a flow.

So, I have been exporting model factory functions instead that my other services can use to build the models. But I am not sure how to use this named type workaround with a factory, since I have to define the types inside of the factory function which makes it a private type.

Any thoughts?

@Bnaya
Copy link
Member

Bnaya commented Nov 21, 2019

but my models include flows which don't work for me when compiling to JS and importing into another library. I get a "Action requires parent context" error for any action called involving a flow.

I don't think it should happen.
Maybe you have 2 versions of mobx/mobx-state-tree?

@skokenes
Copy link
Author

skokenes commented Nov 21, 2019

I'll look into that. I put together a sample repo showing this problem in a new issue if you're interested in taking a look and seeing if there is anything obviously wrong that I am doing with my setup #1427

@beaulac
Copy link

beaulac commented Nov 23, 2019

I was having the same problem with union types, but I had some success by trying defining the output of the union using the other interfaces. union() is variadic and so has some crazy return types, but to keep it simple I looked at the one for 2 models:

ITypeUnion<
  ModelCreationType2<PA, FCA> | ModelCreationType2<PB, FCB>,
  ModelSnapshotType2<PA, FSA> | ModelSnapshotType2<PB, FSB>,
  ModelInstanceType<PA, OA> | ModelInstanceType<PB, OB>
    >

So in order to leverage the predefined interfaces, I built upon your method above with the following:

const ModelAInferred = t.model({/*...*/});

type ModelAFactoryType = typeof ModelAInferred;
interface ModelAFactoryInterface extends ModelAFactoryType {}

export const ModelA: ModelAFactoryInterface = ModelAInferred;

interface IModelACreationType extends SnapshotIn<ModelAFactoryInterface> {}
interface IModelASnapshot extends SnapshotOut<ModelAFactoryInterface> {}
interface IModelAStore extends Instance<ModelAFactoryInterface> {}


const ModelBInferred = t.model({/*...*/});

type ModelBFactoryType = typeof ModelBInferred;
interface ModelBFactoryInterface extends ModelBFactoryType {}

export const ModelB: ModelBFactoryInterface = ModelBInferred;

interface IModelBCreationType extends SnapshotIn<ModelBFactoryInterface> {}
interface IModelBSnapshot extends SnapshotOut<ModelBFactoryInterface> {}
interface IModelBStore extends Instance<ModelBFactoryInterface> {}


interface ModelABUnionFactoryInterface extends ITypeUnion<
IModelACreationType | IModelBCreationType,
IModelASnapshot | IModelBSnapshot,
IModelAStore | IModelBStore
> {}

export const ModelABUnion: ModelABUnionFactoryInterface = t.union(ModelA, ModelB);

type ModelABUnionStore = IModelAStore | IModelBStore;

It worked pretty well; I have a union model that is part of another model, and with this change I significantly reduced the size of type declarations:

  • UnionModel.d.ts from 813 lines to 302 lines
  • UnionModelConsumer.d.ts 894 lines to 78 lines (wow)

Probably this would also work just as well for >2 model unions (haven't tried), by just adding them to

interface ModelABUnionFactoryInterface extends ITypeUnion<
ModelACType | ModelBCType | ModelCCType | /* ... */,
IModelASnapshot | IModelBSnapshot | ModelCSnapshot | /* ... */,
IModelAStore | IModelBStore | ModelCStore | /* ... */,
> {}

Definitely pretty verbose, but, again, better than the alternative 😅

@Bnaya
Copy link
Member

Bnaya commented Nov 23, 2019

Interesting!
I will try to look into that aswell

@Bnaya
Copy link
Member

Bnaya commented Nov 24, 2019

I think i have a nice solution, but i need help with validation before we can make it into a PR
basically i'm wrapping types.union calls with a function that defer the extracting of the internal model shape.

function lazyInferenceTypeUnion<
  M1 extends IAnyModelType,
  M2 extends IAnyModelType,
  M3 extends IAnyModelType,
>(m1: M1, m2: M2, m3: M3): LazyInferenceModelType<M1> | LazyInferenceModelType<M2> | LazyInferenceModelType<M3>  {
  return t.union(m1, m2, m3);
}

type LazyInferenceModelType<T extends IAnyModelType> = IType<ExtractProps<T>, ExtractOthers<T>, ExtractCSTWithoutSTN<T>>;

See diff:
skokenes/mst-typescript-dec-test@495a3c6

which leads to:

export declare const ModelD: LazyInferenceModelType<ModelAFactoryInterface> | LazyInferenceModelType<ModelBFactoryInterface> | LazyInferenceModelType<ModelCFactoryInterface>;
declare type LazyInferenceModelType<T extends IAnyModelType> = IType<ExtractProps<T>, ExtractOthers<T>, ExtractCSTWithoutSTN<T>>;

@Bnaya
Copy link
Member

Bnaya commented Nov 24, 2019

I have a variadic variant of that function, but i'm not sure if its 100% correct types:

function lazyInferenceTypeUnionVariadic<
  ARGS extends Array<IAnyModelType>
>(...args: ARGS): LazyInferenceModelType<ARGS[number]>  {
  return t.union(...args);
}

@Bnaya
Copy link
Member

Bnaya commented Nov 25, 2019

Hey @xaviergonz I would like your feedback for these union wrappers,
They simplify the emitted declarations for unions on some cases
I've also wrote variadic variants that will not require overloads
If you don't see any inherent issue with them, i will try to make a PR and add it as overload, or maybe alias for union with different name and type
Bnaya/mst-typescript-dec-test@d47aeaa

import {
  types as t,
  Instance,
  _NotCustomized,
  IType
} from "mobx-state-tree";
import { ExtractProps, IAnyModelType, ExtractOthers, ExtractCSTWithoutSTN } from "mobx-state-tree/dist/internal";

type ForDirectExtend<T> = T;

const _ModelA = t.model({
  foo: t.string
});

interface ModelAFactoryInterface extends ForDirectExtend<typeof _ModelA> {}
export const ModelA: ModelAFactoryInterface = _ModelA;

const _ModelB = t.model({
  bar: t.number
});

interface ModelBFactoryInterface extends ForDirectExtend<typeof _ModelB> {}
export const ModelB: ModelBFactoryInterface = _ModelB;

export const _ModelC = t.model({
  a: ModelA
});

interface ModelCFactoryInterface extends ForDirectExtend<typeof _ModelC> {}
export const ModelC: ModelCFactoryInterface = _ModelC;

const _ModelD = lazyInferenceTypeUnion(ModelA, ModelB, ModelC);
const variadicUnion = lazyInferenceTypeUnionVariadic(ModelA, ModelB, ModelC);

export const ModelD = _ModelD;

type IT = Instance<typeof variadicUnion>;
declare const bla: IT;

if ("foo" in bla) {
  bla.foo.anchor
}

function lazyInferenceTypeUnion<
  M1 extends IAnyModelType,
  M2 extends IAnyModelType,
  M3 extends IAnyModelType,
>(m1: M1, m2: M2, m3: M3): LazyInferenceModelType<M1> | LazyInferenceModelType<M2> | LazyInferenceModelType<M3>  {
  return t.union(m1, m2, m3);
}

function lazyInferenceTypeUnionVariadic<
  ARGS extends Array<IAnyModelType>
>(...args: ARGS): LazyInferenceModelType<ARGS[number]>  {
  return t.union(...args);
}

type FilterOnly<T, N> = T extends N ? T : never;

function lazyInferenceTypeUnionVariadic3<
  ARGS extends Array<IAnyModelType>
>(...args: ARGS): ({ [P in FilterOnly<keyof ARGS, number>]: LazyInferenceModelType<ARGS[P]> } )[number] {
  return t.union(...args);
}

type LazyInferenceModelType<T extends IAnyModelType> = IType<ExtractProps<T>, ExtractOthers<T>, ExtractCSTWithoutSTN<T>>;

const result_lazyInferenceTypeUnionVariadic = lazyInferenceTypeUnionVariadic(ModelA, ModelB, ModelC);
const result_lazyInferenceTypeUnionVariadic3 = lazyInferenceTypeUnionVariadic3(ModelA, ModelB, ModelC);

Bnaya added a commit that referenced this issue Aug 2, 2020
@Bnaya
Copy link
Member

Bnaya commented Mar 5, 2021

Worth noting,
typescript 4.2 has feature that i believe will drastically improve our use-case: Smarter Type Alias Preservation
https://devblogs.microsoft.com/typescript/announcing-typescript-4-2/#smarter-type-alias-preservation

m3zn added a commit to cgi-italy/oida that referenced this issue Dec 16, 2021
Reduce size of composite mst types by exporting types interface
See mobxjs/mobx-state-tree#1425 for details
@coolsoftwaretyler
Copy link
Collaborator

Hey folks - it looks like we decided not to merge the proposed solution directly into MST, but it can be implemented by consuming apps, as per the discussion in #1562.

Since it's been a while without activity here, I'm going to close out this issue. Thanks for all the good work here!

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

6 participants