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

Idea: install helper #128

Closed
staltz opened this issue Oct 6, 2016 · 9 comments
Closed

Idea: install helper #128

staltz opened this issue Oct 6, 2016 · 9 comments

Comments

@staltz
Copy link
Owner

staltz commented Oct 6, 2016

install(debounce) would monkey patch Stream so you could do a$.debounce(300).

@xtianjohns
Copy link
Contributor

Would the primary use of the installation helper be to make extras easier to implement for library consumers? If so, would it make sense for the helper to be attached to the extra?

import xs from 'xstream';
import debounce from 'xstream/extra/debounce';

// install the extra
debounce.install();

// use the extra
xs.of( 'a' ).debounce(300);

Alternatively, the function could be exported from core.

import xs, { install } from 'xstream';
import debounce from 'xstream/extra/debounce';

// install the extra
install( debounce );

// use the extra
xs.of( 'a' ).debounce(300);

The former doesn't lend itself to customization (pass your own operators to install). The latter does, and for that reason we'd need to do some name/symbol checking.

import xs, { install } from 'xstream';

function mapTo( ) { return 1; }

// install the extra
install( mapTo ); // throw here? do nothing?

// okay, it threw. let's give it a name that it likes
function debounce( ) { return 1; }

install( debounce ); // what now?

@staltz
Copy link
Owner Author

staltz commented Nov 18, 2016

If so, would it make sense for the helper to be attached to the extra?

That's interesting, but ...

Alternatively, the function could be exported from core.

I prefer this option, because

The former doesn't lend itself to customization (pass your own operators to install). The latter does

And

install( mapTo ); // throw here? do nothing?

I think it should do nothing. If we put install in core, it has to be super super small and simple. People doing install(flatten) are abusing the API.

One thing I haven't considered is how to support TypeScript. I wish there would be a way, but I'm afraid there won't be. :/

By the way, perhaps xs.install is nicer than import {install} from 'xstream'.

@wclr
Copy link
Contributor

wclr commented Nov 28, 2016

What is about typings?

@geovanisouza92
Copy link

I think I found a way, based on official docs using "interface merging" and mixins:

  • we create an interface Stream and change the current Stream to implement Stream interface;
  • each extra declare an extension to that Stream interface and a basic implementation, that is applied as a mixin on Impl prototype;

Example:

// The Stream interface and core implementation

export interface Stream {
  map(fn: Function): Stream
}

export class Impl {
  map(fn: Function): Stream {
    // ...
  }
}

export function create(producer: object): Stream {
  return new Impl()
}
// debounce extra

import { Impl } from './stream'
import { applyMixin } from './mixin'

// Define the partial interface
declare module './stream' {
  interface Stream {
    debounce(time: number): Stream
  }
}

// Partial implementation (aka Mixin)
class Debounceable {
  debounce(time: number): Debounceable {
    return null
  }
}

// Modify Impl prototype
applyMixin(Impl, [Debounceable])
// Mixin helper, but could be replaced with direct prototype manipulation

export function applyMixin(derivedCtor: any, baseCtors: any[]) {
  baseCtors.forEach(baseCtor => {
    Object.getOwnPropertyNames(baseCtor).forEach(name => {
      derivedCtor.prototype[name] = baseCtor.prototype[name]
    })
  })
}

Usage:

import {Stream, create} from './stream'

var s = create(null)
s.map // OK
s.debounce // Error, Stream does not have "debounce"

// ...
import './debounce'

s.debounce // OK

P.S.: I'm not sure how the MemoryStream fits here.

@wclr
Copy link
Contributor

wclr commented Oct 29, 2017

It is enough to add in each extra operator module:

declare module '../index' {
    interface Stream<T> {
        dropRepeats<T>(isEqual?: ((x: T, y: T) => boolean) | undefined): Stream<T>
    }
}

Stream.prototype.dropRepeats = function (isEqual: any): any {
  return this.compose(dropRepeast(isEqual))
}

There is small consistency problem with interface merging that it will available even if one doesn't import the extra operator in a particular module. One just has to have it imported in a project scope somewhere, and merged interface will be available all over the place. Though this is how it works, and probably should work this way.

@wclr
Copy link
Contributor

wclr commented Oct 29, 2017

I've created a PR #227
Need to review the approach.

@staltz
Copy link
Owner Author

staltz commented Oct 31, 2017

@whitecolor I just saw your PR #227, thanks. The declare module approach is good, but the implicit prototype monkey patching occurring during import is dangerous as it's "magic" to a library user. Originally we were discussing about an explicit install helper.

As the latest discussion in #224, the plan is to keep the current folder structure, keep compose-based workflow for extras, but simply import&export from within index.ts.

@wclr
Copy link
Contributor

wclr commented Nov 18, 2017

Have made a package xstream-extra for this.

@staltz
Copy link
Owner Author

staltz commented Jan 30, 2018

I think this issue won't happen.
I'm looking forward to a pipe-style API more than dot-chaining. It's also better for predictability and sharing community operators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants