Skip to content

Proposed type tweak to improve use of Record instance in TypeScript code #341

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

Closed
andrewdavey opened this issue Feb 23, 2015 · 28 comments
Closed

Comments

@andrewdavey
Copy link

When using records in TypeScript, it would be nice to define an interface declaring the properties. We can then access those properties, instead of going through the get method. This cleans up code and catches typos at compile time.

interface Customer extends Map<string, any> {
  id: string;
  name: string;
}

var CustomerRecord = Immutable.Record<Customer>({ id: "", name: "" });

var customer1 = new CustomerRecord({ id: "1", name: "John" });

alert(customer1.id);
customer1 = customer1.set("name", "Jon"); // Can still use as a Map
alert(customer1.wrong); // Compile-time error here

To achieve this, the Record function and interface declaration need to be made generic:

export module Record {
  export interface Class<T extends Map<string, any>> {
    new (): T;
    new (values: { [key: string]: any }): T;
    new (values: Iterable<string, any>): T; // deprecated
  }
}

export function Record<T extends Map<string, any>>(
  defaultValues: { [key: string]: any }, name?: string
  ): Record.Class<T>;

Thoughts?

@leebyron
Copy link
Collaborator

Great idea. I'll look into this more

@andrewdavey
Copy link
Author

I just realised a problem. Calls to .set, etc, return a Map<string, any> not the subtype. To fix I created a new interface to extend from.

interface TypedMap<T extends Map<string, any>> {
  set(key: string, value: any): T;
  // ...plus all the other methods of Map...
}

Then

interface Customer extends TypedMap<Customer> {
  ...
}

A bit hacky, but seems to work!

@leebyron
Copy link
Collaborator

There's a missing feature from typescript that would help solve this. The idea of the return type being "this" eg the same type as the context object. In the meantime yes you have to manually write it out for any subtype which is pretty awful

@richardhuaaa
Copy link

That would be this issue, which would allow functions on a base interface to return the type of its derived interface:
microsoft/TypeScript#285

However, I think the following issue, which would allow you to extend a generic type, would help the Record interface be defined significantly better than the current approach:
microsoft/TypeScript#2225

Then you can do:

interface RecordClass<T> extends T, Map<Set, Any> {
}

Where RecordClass (excuse the naming) would allow you to call all of the functions you expect on a record (such as .set(), etc) while still allowing you to access all of the properties in T using dot notation. Then the rest of the interface would look as follows:

export module Record {
  export interface Class<T> {
    new (): RecordClass<T>;
    new (values: T): RecordClass<T>;
  }
}

export function Record<T>(
  defaultValues: T, name?: string
  ): Record.Class<T>;

This would be nicer than the proposed approach because the defaultValues and values arguments can now be type-checked against T, the 'Customer' interface shown in the first comment no longer needs to extend anything, and the resulting code will be clearer as all Record types are explicitly marked as such.

@Keats
Copy link

Keats commented Jul 25, 2015

@andrewdavey did you end up using typed maps instead of records as in your last post? Using maps kinda gets rid of the benefits of types as you have to use a string with .get and set (too bad javascript can't overload that)
Your first post looks like a nicer solution to me.

@ghost
Copy link

ghost commented Aug 4, 2015

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

@aindlq
Copy link

aindlq commented Oct 12, 2015

With typescirpt 1.6 and intersection types (see microsoft/TypeScript#3622) it is possible to significantly improve typings for Records. Here is simple POC:

typings

  export module Record {
    type IRecord<T> = T & TypedMap<T>;

    interface TypedMap<T> extends Map<string, any> {
      set(key: string, value: any): T & TypedMap<T>
    }

    interface Class<T> {
      new (): IRecord<T>;
      new (values: T):  IRecord<T>;

      (): IRecord<T>;
      (values: T): IRecord<T>;
    }
  }

  export function Record<T>(
    defaultValues: T, name?: string
  ): Record.Class<T>;

test

import { Record } from 'immutable';

const MyType = Record({a:1, b:2, c:3});

var t1 = new MyType();
const a = t1.a;

// type error
const z = t1.z;

const t2 = t1.set('a', 10);
const a2 = t2.a;

// type error
const z2 = t2.z;

const t3 = t2.clear();

Also polymorphic this type will be available in 1.7, it is already in master (see microsoft/TypeScript#4910)

@jtmueller
Copy link

@aindlq - That's fantastic! By renaming the "Class" interface to avoid naming conflicts with the existing immutable.d.ts, I was able to include your sample in an "immutable-overrides.d.ts" file without having to modify the provided immutable.d.ts and then maintain my customizations.

/// <reference path='../../node_modules/immutable/dist/immutable.d.ts'/>

declare module Immutable {
    export module Record {
        type IRecord<T> = T & TypedMap<T>;

        interface TypedMap<T> extends Map<string, any> {
            set(key: string, value: any): IRecord<T>
        }

        interface Factory<T> {
            new (): IRecord<T>;
            new (values: T): IRecord<T>;

            (): IRecord<T>;
            (values: T): IRecord<T>;
        }
    }

    export function Record<T>(
        defaultValues: T, name?: string
    ): Record.Factory<T>;
}

More importantly, it lets me do incredibly convenient things like this. Thanks!

/// <reference path='../../typings/immutable/immutable-overrides.d.ts'/>
import Immutable = require('immutable');

export interface Todo {
  id?: number;
  text: string;
  completed: boolean;
};

export type TodoList = Immutable.List<Immutable.Record.IRecord<Todo>>;

export const TodoRecord = Immutable.Record<Todo>({ text:'', completed: false, id: -1 }, "Todo");

@OliverJAsh
Copy link

Could we get these new type definitions for Record added and documented?

@dimitrikochnev
Copy link

This type definitions cause another problem with extending:

interface ModelAttrs {
  id: string;
}

class Model extends Record<ModelAttrs>({ id: null }) {
  ...
}

Error: error TS2509: Base constructor return type 'ModelAttrs & TypedMap<ModelAttrs>' is not a class or interface type.

@dimitrikochnev
Copy link

Record inheritance works since TypeScript 1.6 has been released. #166

@dimitrikochnev
Copy link

With extending expressions from 1.6 and polymorphic this from 1.7 the temporary typings can be:

declare module Immutable {
  export function Record<T>(defaultValues: T, name?: string): Record.Factory<T>;

  export module Record {
    interface Base extends Map<string, any> {
      set(key: string, value: any): this;
      // ...
    }

    interface Factory<T> {
      new (): Base;
      new (values: T): Base;
      (): Base;
      (values: T): Base;
    }
  }
}

Before:

interface BaseAttrs {
  id: string;
}

type BaseRecord = Record.IRecord<BaseAttrs>;

interface TodoAttrs extends TodoAttrs {
  name: string;
}

const TodoRecord = Record<TodoAttrs>({
  id: null,
  name: null
})

type Todo = Record.IRecord<TodoAttrs>;

// no support for custom methods

function foo<T extends BaseRecord>(m: T) {
  // ...
}

function bar(todo: Todo) {
  // ...
}

var m = new TodoRecord(...)

m.name; // ok

foo(m); // ok
bar(m); // ok

After:

interface BaseAttrs {
  id: string;
}

type BaseRecord = BaseAttrs & Record.Base;

interface TodoAttrs extends TodoAttrs {
  name: string;
}

class Todo extends Record<TodoAttrs>({ id: null,name: null }) implements TodoAttrs {
  id: string;
  name: string;

  // some additional methods

  lowerName(): string {
    return this.name.toLowerCase();
  }
}

// type alias isn't required for Todo

function foo<T extends BaseRecord>(m: T) {
  // ...
}

function bar(todo: Todo) {
  // ...
}

var m = new Todo(...)

m.name; // ok
m.lowerName(); //works
m = m.set("name", "...");
m.lowerName(); // still works

foo(m); // ok
bar(m); // ok

The bad moment in this approach is that we should explicitly declare properties in a record class.

@Keats
Copy link

Keats commented Dec 10, 2015

Should we at least make a PR for an updated .d.ts for use in typescript 1.7 ?

@myitcv
Copy link

myitcv commented Dec 15, 2015

@Keats - the challenge is that upgrading the type definition it immediately loses backwards compatibility with earlier TypeScript versions pre 1.7. So the version of the type definition that ships with the package will have to be compatible with the earliest version of TypeScript we want to support.

See this related discussion. I'm going to try and get a typings version of the ImmutableJS type definition up and running soon that is appropriate for 1.7 (and another with the changes introduced in 1.8)

@jessep
Copy link

jessep commented Jan 5, 2016

In the proposed Record definition above, one can't supply only some of the properties to the Record Factory. Records are supposed to fill in missing defaults, so this doesn't seem ideal.

The following line from the definition of Record.Factory makes it so that we have to specify all required properties when creating a new with a record factory.

new (values: T): IRecord<T>;

For example, the following has a type error of Property 'b' is missing in argument of type '{a: number}'

import * as Immutable  from 'immutable';

interface Test {
  a: number,
  b: string
}

const TestFactory = Immutable.Record<Test>({a: 2, b: 'hiya'});
const TestRecord = TestFactory({a: 10});
// There's now an error in the call to TestFactory that says
// "Property 'b' is missing in argument of type '{a: number}'

We can make the Record.Factory take {values: any} and that gets rid of the error, but I'm not sure if that's desired. It does seem that this would fit the actual behavior of Immutable.js, because its record class just discards any undefined properties passed to the constructor. For example:

const Immutable = require('immutable');

const TestRecordFactory = Immutable.Record({apple: 'yum', potato: 'meh'}, 'TestRecord');
const testRecord = TestRecordFactory({somethingElse: 'wah?'});
console.log(testRecord.toJSON())
// => {apple: 'yum', potato: 'meh'}

I'm new to typescript and static typing in general, so I have no idea about any of this. Any ideas on how to make the new Record definition support this part of the Immutable Record api?

The new definition with any would be:

/// <reference path='../../node_modules/immutable/dist/immutable.d.ts'/>

declare module Immutable {
    export module Record {
        type IRecord<T> = T & TypedMap<T>;

        interface TypedMap<T> extends Map<string, any> {
            set(key: string, value: any): IRecord<T>
        }

        interface Factory<T> {
            new (): IRecord<T>;
            new (values: any): IRecord<T>;

            (): IRecord<T>;
            (values: any): IRecord<T>;
        }
    }

    export function Record<T>(
        defaultValues: T, name?: string
    ): Record.Factory<T>;
}

@OliverJAsh
Copy link

Is it possible to have something like a TypeScript interface but using immutable-js? I don't think Record is exactly the same because it requires default values. I don't want default values, I just want an immutable object defined by an interface. At least I think that's what I want.

@myitcv
Copy link

myitcv commented Jan 12, 2016

@OliverJAsh - we use code generation of classes that are backed by ImmutableJS data structures to achieve what I believe you're referring to. i.e. we first define the structure we want (what you're referring to as an interface), then we code generate the actual implementation.

@OliverJAsh
Copy link

I'm not sure I understand. Do you have an example?

On Tue, 12 Jan 2016, 16:32 Paul Jolly [email protected] wrote:

@OliverJAsh https://github.com/OliverJAsh - we use code generation of
classes that are backed by ImmutableJS data structures to achieve what I
believe you're referring to. i.e. we first define the structure we want
(what you're referring to as an interface), then we code generate the
actual implementation.


Reply to this email directly or view it on GitHub
#341 (comment)
.

@SH4DY
Copy link

SH4DY commented Jan 13, 2016

@OliverJAsh yes, thats exactly what I'm looking for as well. @myitcv can you explain what you mean? I can't imagine how you generate classes with ImmutableJS as the base.

@jmreidy
Copy link

jmreidy commented Jan 26, 2016

@SH4DY I'm assuming @myitcv uses something like s-panferov/tsimmutable

@myitcv
Copy link

myitcv commented Jan 30, 2016

@jmreidy thanks for the link, I'll give that a closer look.

@SH4DY from a brief look at tsimmutable there is some similarity to our approach. tsimmutable uses TypeScript interface definitions as the basis for its code generation (e.g. the Profile and User interfaces in the examples). We use Protocol Buffers.

Fundamentally however it doesn't matter what your starting point is, just so long as you can describe the structure of your types.

So let's take the Profile example from the tsimmutable project:

export interface Profile {
    firstName: string;
    lastName: string;
}

From this we code generate a class with the following interface (you can see how we adopt a similar pattern to that used on the ImmutableJS containers):

export declare class Profile implements IUnmarshaler {
    static NewProfile(): Profile;
    static NewFromJSON(json: Object): Profile;
    SetFirstName(vFirstName: string): Profile;
    FirstName(): string;
    SetLastName(vLastName: string): Profile;
    LastName(): string;
    toString(): string;
    AsMutable(): Profile;
    AsImmutable(): Profile;
    WithMutations(cb: (a: Profile) => void): Profile;
}

Notice that with the merging of microsoft/TypeScript#6532 it is possible to translate the accessor methods to readonly getters

The underlying implementation behind each such class uses an ImmutableJS Map<string, Object> to store the value of each field (firstName and lastName in this example). We did this principally for reasons of efficiency (memory and CPU) but to be honest we never got round to quantifying this statement so take it with a large pinch of salt!

The simplest underlying implementation however would be a plain object based approach, e.g. (playground link:

class Profile {
    private firstName: string;
    private lastName: string;

    FirstName(): string {
        return this.firstName;
    }

    SetFirstName(vFirstName: string): Profile {
        let res: Profile = shallowCopy(this); 
        res.firstName = vFirstName;
        return res;
    }

    LastName(): string {
        return this.lastName;
    }

    SetLastName(vLastName: string): Profile {
        let res: Profile = shallowCopy(this); 
        res.lastName = vLastName;
        return res;
    }   
}

function shallowCopy<T>(v: T): T {
    let res = new Object();
    for (let i in v) {
        res[i] = v[i];
    }
    return <T>res;
}

let p1 = new Profile();
let p2 = p1.SetFirstName("Paul");
let p3 = p2.SetLastName("Jolly");
console.log(p1, p2, p3, p1 === p2, p1 === p3);

The efficiency of this plain and simple approach vs an ImmutableJS approach will depend on many factors: how many mutations you perform, size (in terms of number of fields) of classes.... the list goes on. Again, measuring would be the only concrete way to demonstrate efficiency one way or the other.

We'll likely be sharing some of our tooling around this in the next couple of months, so I'll try to remember to update this thread in case anyone is interested.

@danielearwicker
Copy link
Contributor

@jessep - to make your properties optional, change your interface declaration:

interface Test {
  a?: number,
  b?: string
}

@danielfigueiredo
Copy link

danielfigueiredo commented Jul 26, 2016

@Silhouettes I've done something very similar that handles the usage of Immutable records with TypeScript interfaces
@andrewdavey @OliverJAsh this works in a clean way while we don't have immutable Records working smoothly with typescript
https://github.com/rangle/typed-immutable-record
the repo has code with examples, it's a very tiny library and the only downside since I'm just wrapping around immutable is that you need two interfaces, but I'm using it everywhere and it provides a huge benefit.

@aalpgiray
Copy link

aalpgiray commented Sep 7, 2016

I found a class to get prop name as string array, then implemented a SetValue Method this has some rough edges.

export class NavigableObject<T>{
    constructor(private obj: T, private path: string[] = []) { }

    To<R>(p: (x: T) => R): NavigableObject<R> {

        let propName = this.getPropName(p)

        if (propName) {
            return new NavigableObject<R>(
                p(this.obj),
                this.path.concat(propName)
            );
        } else {
            return new NavigableObject<R>(
                p(this.obj),
                this.path
            );
        }
    }

    getPath() {
        return this.path;
    }


    private static propertyRegEx = /\.([^\.;]+);?\s*\}$/;

    private getPropName(propertyFunction: Function) {
        let value = NavigableObject.propertyRegEx.exec(propertyFunction.toString())
        if (value)
            return value[1];
    }
}

function NavigatableRecordFactory<X>(defaultValues: X, name?: string) {
    abstract class NavigatableRecord<P extends NavigatableRecord<P>> extends Record(defaultValues, name) {
        SetValue<T>(fn: (x: NavigableObject<P>) => NavigableObject<T>, value: T) {
            return this.setIn(fn(new NavigableObject<any>(this)).getPath(), value)
        }
    }
    return NavigatableRecord;
}

interface IUSER {
    Name: string;
    Age: number;
}

export class USER extends NavigatableRecordFactory<IUSER>({
    Name: "Simy the bothless",
    Age: 27,
})<USER> implements IUSER {
    Name: string;
    Age: number;
}

and then use it like

state.Name // works

state.SetValue(t => t.To(q => q.Name), "test string") // typecheks
state.SetValue(t => t.To(q => q.Name), 123) // error

it also works with nested properties

somenestedImmutable.SetValue(t =>t.To(q => q.Date).To(q => q.Time), 213213123)

but cant get it work without needing to implement method in child class ,
little help would be nice if it is possible :)

Typescript v.2.1.0-dev20160805

@danielearwicker
Copy link
Contributor

@aalpgiray looks like a syntax error in somenestedImmutable.(t => ... ?

@leebyron
Copy link
Collaborator

leebyron commented Mar 8, 2017

This should be fixed in master, will be released soon

@leebyron leebyron closed this as completed Mar 8, 2017
@born2net
Copy link

born2net commented May 4, 2017

fyi I tried with latest release immutable beta rc and no luck


interface ITimelineState {
    zoom: number;
    duration: number;
    channels: Array<IChannels>;
    outputs: Array<IOutputs>;
    items: Array<IItem>;
}

let state: Map<any, ITimelineState> = Map({ // <<<< ERROR
        zoom: 1,
        duration: 500,
        channels: [],
        outputs: [],
        items: []
    });

@18steps
Copy link

18steps commented May 21, 2017

4.0.0-rc.2 as issues in itself atm, e.g.

ERROR in .../node_modules/immutable/dist/immutable-nonambient.d.ts (2331,22): Interface 'Keyed<K, V>' cannot simultaneously extend types 'Seq<K, V>' and 'Keyed<K, V>'.
Named property 'size' of types 'Seq<K, V>' and 'Keyed<K, V>' are not identical.

ERROR in .../node_modules/immutable/dist/immutable-nonambient.d.ts (2441,22): Interface 'Indexed' cannot simultaneously extend types 'Seq<number,T>' and 'Indexed'. Named property 'size' of types 'Seq<number, T>' and 'Indexed' are not identical.

etc.

3.8.1 also has issues with types on Records for .set

const SectionRecord = Record(createInitialState());
export class Section extends SectionRecord implements ISectionState {
    defaultHypo: DefaultHypo;
...
}
...
const newSection = section.set('defaultHypo', newDefault);

newSection here is actually not a new instance of SectionRecord, but a Map<string, any>. Same issue for all my typed Records.

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