Skip to content

Module composition streategy proposal #643

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

Closed
wants to merge 1 commit into from

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Nov 3, 2015

This PR intended to show proof-of-concept of proposal, neither mature or intended to be actually merged.

While seeing issues like #635, #642, came to feel if there's way to create module met requirements like

  • type definition to be preserved over core, extended along with any custom observable module created in further
  • reduce type signature duplication over difference places

This proposal is based on Mixin of typescript, compose class implementations to extend modules if necessary.

How composition works

Attached PR illustrates rough mechanisms of composition. First, there is Core implementation of observable having bare minimum set in CoreObservable.ts. In this PR it implements interface of CoreOperator, this could be ambient merging introduced in typescript nightly. (There's one caveat of ambient merging, mixin class should declare all interfaces even if it's not implemented in class)
Rx.Core.ts does exports it directly without any extension.

ExtendedObservable.ts represents fully features set of Observable module, corresponds to current 'KitchenSink'. it does implements additional operator. Rx.Extended.ts is place where actual composition occurs, it declares mixin Observable<T> which implements Core & Extended both, then call applyMixin to compose mixin class at runtime. This runtime composition occurs very first time module ExtendedObservable is imported.

Rx.My.ts is representing cases where any consumer would like to create custom module for own flavor. In this PR, it utilizes core observable along with adding new operator implementation.

testIndex.ts shows usage of each module, simply import it and type definition starts to work.

Declaring Mixin

When declaraing mixin, it does do some specifics

  1. should include all factory function signatures like of, lift, etcs
    : Actual implementation of factory functions are injected while applying mixin (line 18 in CoreObservable) it is important to declare signature in each mixin explicitly. This prevents issues like Type definition for extended operator is not published #642, extended observale generator to return extended type to allow correct type inference.
  2. Mixin operator's stub declaration is property, not function. And it does have 'different' return type than original.
    : For example. CoreObservable::filter():Operators should be declared in mixin as MyObservable::filter():MyObservable. This is for type inference as same as 1.

Possible pros

  • explicit prototype injection in module declaration can go away (though mixin internally does this)
  • type definition works for all cases (core, extended, even custom)

Cons

  • Mixin declaration still requires some of stubs though it's reduced compare to current approach.
  • Requires specific formats to declare mixin to make it work, such as property stub declaration with corresponding types.

@benlesh
Copy link
Member

benlesh commented Nov 5, 2015

I think I'll personally need to let this idea simmer for a while. Since TypeScript and Babel are now both going to support the :: operator, and we have a two globals that handle the rest, I'm inclined to think that TypeScript adding :: solved our problem for us for most people... Hmmm...

@kwonoj
Copy link
Member Author

kwonoj commented Nov 5, 2015

@Blesh agreed. This is one of proposals, and also could possibly write up another using new operator. BTW, :: in typescript is not landed yet, right? When I checked issues for that some days ago it seems it was ongoing.

@benlesh
Copy link
Member

benlesh commented Nov 6, 2015

:: in typescript is not landed yet, right?

That's correct. It's sitting in a development branch last I looked.

@kwonoj
Copy link
Member Author

kwonoj commented Nov 13, 2015

I came to think this PR can be closed for now, PR can be reopened anytime when time comes to evaluate this vs. any other possibilities.

@benlesh
Copy link
Member

benlesh commented Nov 18, 2015

@jeffbcross is also proposing an alternative to this here: #750

@kwonoj
Copy link
Member Author

kwonoj commented Nov 20, 2015

After discussion with @Blesh , I'm closing this issue for now since alternative candidate #750 seems quite feasible solution. PR can be recreated anytime if this approach need to be revisited.

@lock
Copy link

lock bot commented Jun 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants