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

WIP: Improved types, Fixes #64, #56, #9 #65

Merged
merged 36 commits into from
Mar 28, 2017
Merged

WIP: Improved types, Fixes #64, #56, #9 #65

merged 36 commits into from
Mar 28, 2017

Conversation

mweststrate
Copy link
Member

@mweststrate mweststrate commented Mar 24, 2017

  • go from A() to A.create() (better typescript support, easier internal factory construction / typing)
  • improve basic type inference
  • kill action
  • fix identifiers
  • enable strict null checks
  • verify types of all factories (including non-nullable of snapshots and types)
  • merge Type & Factory
  • update readme

@mweststrate
Copy link
Member Author

This MR also merges #59 and #61

),
type: this,
isFactory: true,
factoryName: this.name,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really needed? Why not use this.type.name instead? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, left it as is for now, will factor out later

@@ -69,3 +70,8 @@ export abstract class ComplexType extends Type {
return this.isValidSnapshot(value)
}
}

export function typecheck(type: IType, snapshot: any) {
if (!type.is(snapshot))
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: Maybe on production build this could be removed to speedup execution, but fails with errors?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, see also #62

x: types.number,
y: types.maybe(types.string),
method() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: Also add a computed property and a manual test for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 3ad167a on improved-types into ** on master**.

@@ -163,7 +163,7 @@ export class Node {

return fail(`Cannot add an object to a state tree if it is already part of the same or another state tree. Tried to assign an object to '${this.path}/${subpath}', but it lives already at '${getPath(child)}'`)
}
const existingNode = this.getChildNode(subpath)
const existingNode = this.getChildMST(subpath)
const newInstance = childFactory.create(child)

if (existingNode && existingNode.type === newInstance.factory) {
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: Should'nt be getType(newInstance) on the right side?

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 342b82a on improved-types into ** on master**.

@mweststrate mweststrate mentioned this pull request Mar 27, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling bc7b2fe on improved-types into ** on master**.

@mweststrate
Copy link
Member Author

@mattiamanzati what do you think, ok to merge? Still some todo's left, but I think the current setup is good and nice enough to cut a new release, to no longer postpone all the breaking changed 🙈

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 2d88f1a on improved-types into ** on master**.

@mattiamanzati
Copy link
Contributor

mattiamanzati commented Mar 27, 2017

Uh, I think that identifier should also be changed to return a constant value or to be a generic type. The reason is because the parenthesis are used on a type only if that type is generic.
I think that atm is'nt clear that
types.string("") is different from types.string
and that's why I am so happy about the .create
types.string.create("") will be more explicit and explain that is different from types.string
Since identifier relies only on a isIdentifier: true, would be neat to make that signature generic.

const Todo = t.model({
id: t.identifier(t.string)
})

where identifier() is something like Object.assign({}, type, {isIdentifier: true}).
Typings should be fine because the identifier props will be a factory as before.

@mweststrate
Copy link
Member Author

Great suggestion! Will adjust identifier

@zetoke
Copy link

zetoke commented Mar 28, 2017

Hey, guys!
Do you have estimated date for merging this PR?

@mweststrate
Copy link
Member Author

today or tomorrow I think

@zetoke
Copy link

zetoke commented Mar 28, 2017

@mweststrate Cool! Thank you

@mweststrate mweststrate merged commit 000f854 into master Mar 28, 2017
@zetoke
Copy link

zetoke commented Mar 29, 2017

What is the plan about publish this to NPM?
I'm very excited to refactor my current codebase :D

@mweststrate mweststrate deleted the improved-types branch March 30, 2017 13:58
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

Successfully merging this pull request may close these issues.

None yet

4 participants