phosphorjs / phosphor

The PhosphorJS Library
BSD 3-Clause "New" or "Revised" License
1.04k stars 169 forks source link

Add more type safety to datastore get #420

Open saulshanabrook opened 5 years ago

saulshanabrook commented 5 years ago

This PR makes the datastore class generic with respect to the schemas it was created with. This means that the get method now can verify that the schema you pass is of the same type as the schemas you created the store with.

Note that this should produce no change in emitted code, only in the type declarations.

For example, in the code below, d.get(s2) will fail, because it has a different type than s. We need the <const> type modifier so it types each id as the constant string instead of just a string.

const s = <const>{
  id: "dfd",
  fields: {
    test: new ListField()
  }
};

const s2 = <const>{
  id: "dfd1",
  fields: {
    test: new ListField()
  }
};

const d = Datastore.create({ schemas: [s], id: 0 });

d.get(s2);
d.get(s);
Screen Shot 2019-08-26 at 8 38 35 AM

@vidartf I think you mentioned wanting something like this in one of our earlier meetings, if I am not mistaken.

cc @ian-r-rose @jasongrout

See https://github.com/microsoft/TypeScript/issues/20965 for a discussion of how to get the type of an array, using T[number]

Original ideas was posted here https://github.com/jupyterlab/jupyterlab/issues/5382#issuecomment-520078772

saulshanabrook commented 5 years ago

I also am having a go at passing in a schemas object of { readonly [id: string]: { readonly [name: string]: AnyField } } instead of an array of schemas into the Datastore.create method. That way, we could have type safe get access` where you just pass in the id, instead of the whole schema. However, I am blocked by https://github.com/microsoft/TypeScript/issues/33084

vidartf commented 5 years ago

I also am having a go at passing in a schemas object

This is what I was thinking of previously, but didn't end up researching it. The issue you mentioned was resolved, so does that mean it is possible?

saulshanabrook commented 5 years ago

@vidartf yeah it would just require some larger code changes to the datastores API. Does this PR get you the safety you were looking for?

saulshanabrook commented 5 years ago

I think the issue with T[number] is that it creates a union type of the schemas in unfortunate ways

That's what I would expect. What would you expect the change type to be? A union seems helpful because then in the changed signal you can use branching to handle the different tables and it will be type safe.