-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Comments
Great idea. I'll look into this more |
I just realised a problem. Calls to
Then
A bit hacky, but seems to work! |
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 |
That would be this issue, which would allow functions on a base interface to return the type of its derived interface: 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: 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. |
@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) |
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. |
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) |
@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"); |
Could we get these new type definitions for |
This type definitions cause another problem with extending:
Error: |
Record inheritance works since TypeScript 1.6 has been released. #166 |
With extending expressions from 1.6 and polymorphic
Before:
After:
The bad moment in this approach is that we should explicitly declare properties in a record class. |
Should we at least make a PR for an updated .d.ts for use in typescript 1.7 ? |
@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) |
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 new (values: T): IRecord<T>; For example, the following has a type error of 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 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>;
} |
Is it possible to have something like a TypeScript interface but using immutable-js? I don't think |
@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. |
I'm not sure I understand. Do you have an example? On Tue, 12 Jan 2016, 16:32 Paul Jolly [email protected] wrote:
|
@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. |
@SH4DY I'm assuming @myitcv uses something like s-panferov/tsimmutable |
@jmreidy thanks for the link, I'll give that a closer look. @SH4DY from a brief look at 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 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 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. |
@jessep - to make your properties optional, change your interface declaration: interface Test {
a?: number,
b?: string
} |
@Silhouettes I've done something very similar that handles the usage of Immutable records with TypeScript interfaces |
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 , |
@aalpgiray looks like a syntax error in |
This should be fixed in master, will be released soon |
fyi I tried with latest release immutable beta rc and no luck
|
4.0.0-rc.2 as issues in itself atm, e.g.
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);
|
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.To achieve this, the
Record
function and interface declaration need to be made generic:Thoughts?
The text was updated successfully, but these errors were encountered: