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(union): Union.create() shouldn't ignore extra arguments #151

Merged
merged 2 commits into from
Jun 6, 2017

Conversation

abruzzihraig
Copy link
Contributor

Children in type Union could be any type, currently means it could be either SimpleType or ComplexType so the rest arguments such as environment, parent, subpath shouldn't be ignored in method Union.create().

I would have tried to declare the method create into the type IComplexType such as:

export interface IComplexType<S, T> extends IType<S, T & ISnapshottable<S> & IMSTNode> {
    create(snapshot?: S, environment?: any, parent?: MSTAdministration, subpath?: string): T
}

Then it might be a type IComplexType<any, any>|IType<any, any> or IComplexType<any, any>|ISimpleType<any, any> could be used in the Class Union.

But I'm confused with the current structure in MST:

IComplexType extends IType
ISimpleType extends IType
Type implements IType

So far so good, but why ComplexType extends Type rather than ComplexType extends Type implements IComplexType? It doesn't make sense to me.

Besides, if I use the union type like IComplexType<any, any>|IType<any, any> or IComplexType<any, any>|ISimpleType<any, any>, I wouldn't know how to distinguish them in runtime so that I cannot give it a specific assertion.

That's why finally I just changed the parameter declarations in IType.create().

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 93.093% when pulling cd711b0 on abruzzihraig:master into 58bc205 on mobxjs:master.

@abruzzihraig
Copy link
Contributor Author

Link to #134

@mattiamanzati
Copy link
Contributor

Hi! Thanks for your PR!
The other parameters doesnt appear in IType because we dont want them to be visible, can you please revert only the IType declaration? :)

@abruzzihraig
Copy link
Contributor Author

abruzzihraig commented May 26, 2017

@mattiamanzati if I revert the change in IType, the following lines have to be changed by an assertion as any:

cd711b0#diff-7536e74658295b48b68949f5100a9989R37
cd711b0#diff-7536e74658295b48b68949f5100a9989R45

Is that ok for your code foundation? Using assertion as any here seems it is not a good way... But currently, we cannot use IComplexType or something else to restrict it.

@mattiamanzati
Copy link
Contributor

Its fine for now, soon when all types will be complex by default, that will no more be an issue :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 93.093% when pulling 989dff0 on abruzzihraig:master into 58bc205 on mobxjs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 93.093% when pulling 7a3999e on abruzzihraig:master into 58bc205 on mobxjs:master.

@abruzzihraig
Copy link
Contributor Author

@mattiamanzati Pls check it again, thanks.

@mattiamanzati
Copy link
Contributor

Now its fine! Thanks for your hard work! :)

@mattiamanzati mattiamanzati merged commit fab0623 into mobxjs:master Jun 6, 2017
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.

3 participants