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

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

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

Comments

Projects
None yet
@andrewdavey

andrewdavey commented Feb 23, 2015

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

This comment has been minimized.

Collaborator

leebyron commented Feb 23, 2015

Great idea. I'll look into this more

@andrewdavey

This comment has been minimized.

andrewdavey commented Feb 23, 2015

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

This comment has been minimized.

Collaborator

leebyron commented Feb 24, 2015

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

@silhouettes

This comment has been minimized.

silhouettes commented May 29, 2015

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

jtmueller commented Oct 14, 2015

@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

This comment has been minimized.

OliverJAsh commented Oct 29, 2015

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

@rosendi

This comment has been minimized.

rosendi commented Nov 2, 2015

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.

@rosendi

This comment has been minimized.

rosendi commented Nov 2, 2015

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

@rosendi

This comment has been minimized.

rosendi commented Nov 5, 2015

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

This comment has been minimized.

Keats commented Dec 10, 2015

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

@myitcv

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

OliverJAsh commented Jan 7, 2016

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

This comment has been minimized.

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

This comment has been minimized.

OliverJAsh commented Jan 12, 2016

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

On Tue, 12 Jan 2016, 16:32 Paul Jolly notifications@github.com 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

This comment has been minimized.

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

This comment has been minimized.

jmreidy commented Jan 26, 2016

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

@myitcv

This comment has been minimized.

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

This comment has been minimized.

Contributor

danielearwicker commented Mar 7, 2016

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

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

danielearwicker commented Sep 7, 2016

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

@leebyron

This comment has been minimized.

Collaborator

leebyron commented Mar 8, 2017

This should be fixed in master, will be released soon

@leebyron leebyron closed this Mar 8, 2017

@born2net

This comment has been minimized.

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

This comment has been minimized.

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