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

fix typescript types after type optimization and cast, add castToSnapshot and castToReferenceSnapshot #1074

Merged
merged 12 commits into from
Nov 19, 2018

Conversation

xaviergonz
Copy link
Contributor

@xaviergonz xaviergonz commented Nov 7, 2018

Basically since the fix to make types faster apparently some stuff got broken. This PR fixes:

  • Creation type being downgraded to any in some circumstances
  • cast being broken and basically becoming a fancy "as any"

Additionally:

  • lots of type fixes to by moving IStateTree to the T part of the type rather than to create/model instance type
  • added castToSnapshot to cast models used inside a create method.
  • added castToReferenceSnapshot to cast models to their reference type (string | null) inside a create method
  • cast once more now doesn't compile if used outside an assignment operation
  • reference types snapshots are properly typed as string|number (therefore the new castToReferenceSnapshot)
  • arrays mutate operations (push, splice, etc) now support creation/snapshot types without the need to cast them
  • IComplexType has been removed since now it is equivalent to IType<any, any, ... & IStateTreeNode>

@xaviergonz xaviergonz added the require('@mweststrate') @mweststrate input is required! label Nov 9, 2018
@xaviergonz
Copy link
Contributor Author

@mweststrate thoughts about the addition of castToSnapshot?

@Bnaya
Copy link
Member

Bnaya commented Nov 9, 2018

Maybe we could advice the users to use getSnapshot instead?

@xaviergonz
Copy link
Contributor Author

xaviergonz commented Nov 9, 2018

well, that's what mst does internally, but one of its key features is that it also supports constructs such as this

function f(childModel: SnapshotOrInstance<typeof ChildModel>) {
  self.rootModel = RootModel.create({
    child: castToSnapshot(childModel)
  });
}

to do that otherwise you'd have to either force cast it to as any as SnapshotIn<typeof ChildModel> (which won't tell you if something is wrong)

or with getSnapshot something like

function f(childModel: SnapshotOrInstance<typeof ChildModel>) {
  self.rootModel = RootModel.create({
    child: isNode(childModel) ? getSnapshot(childModel) : childModel
  });
}

which would be unneeded code (and I think won't compile, not 100% sure)

@xaviergonz
Copy link
Contributor Author

Just made sure the .d.ts files generated don't get bigger (spoiler: they don't, they are exactly the same)

@xaviergonz xaviergonz changed the title fix typescript types and cast, add castToSnapshot fix typescript types after type optimization and cast, add castToSnapshot and castToReferenceSnapshot Nov 12, 2018
@xaviergonz xaviergonz self-assigned this Nov 12, 2018
@xaviergonz
Copy link
Contributor Author

@mweststrate pr done

Copy link
Member

@mweststrate mweststrate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xaviergonz changes look good to me! (Granted, my review was quite superficial, as you are now way deeper into the subtleties of the TS type system then I am :)).

One question, did you check whether this negatively impacts the compilation / autocompletion speed, just to not undo the benefits of the previous PR?

Thanks!

@xaviergonz
Copy link
Contributor Author

Yep I did! Quoting myself

Just made sure the .d.ts files generated don't get bigger (spoiler: they don't, they are exactly the same)

@mweststrate
Copy link
Member

mweststrate commented Nov 19, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
require('@mweststrate') @mweststrate input is required!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants