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

feat(): use adapters #198

Closed
wants to merge 3 commits into from
Closed

feat(): use adapters #198

wants to merge 3 commits into from

Conversation

TylorS
Copy link
Member

@TylorS TylorS commented Jan 11, 2016

Uses an adapter interface to allow for cycle-core to adapt
to any and all stream/observable libraries that have an adapter
written. This branch uses an Rx 4 adapter for the tests.

Uses an adapter interface to allow for cycle-core to adapt
to any and all stream/observable libraries that have an adapter
written. This branch uses an Rx 4 adapter for the tests.
@Frikki
Copy link
Member

Frikki commented Jan 11, 2016

👍 Very clever. Code of my own heart. I look forward testing this out. 👍 again!

@TylorS
Copy link
Member Author

TylorS commented Jan 11, 2016

With this cycle-core can be entirely stream/observable library agnostic via adapters.
Currently I have only written an Rx 4 adapter https://github.com/tylors/cycle-rx-adapter
The way this works currently.

import Cycle from '@cycle/core'
import streamAdapter from 'cycle-rx-adapter'

...code here....

run(main, driver, {streamAdapter})

A driver that is not written in the library that the streamAdapater you chose to give to run()
can be coerced into working by adding a streamAdapter property to the driver function.

function driverFn(sink$) {...}

driverFn.streamAdapter = rxAdapter // mostAdapter 

@staltz
Copy link
Member

staltz commented Jan 11, 2016

👍 for the adapter solution in general.

About the API:
Because "just using Cycle with Rx" is going to be far more common than "I want to use Cycle with my own non-officially-supported streams library", an API like

import Rx from 'rx'

run(main, driver, {streamLibrary: Rx})

would be better. In fact, we can assume Rx as the default library. So then we can change package.json

-"cycle-rx-adapter": "^0.2.0",
 "rx": "4.0.7",

It'd make more sense for most Cycle developers to install npm install rx @cycle/core @cycle/dom than to install npm install rx cycle-rx-adapter @cycle/core @cycle/dom.

Also, in the streams adapter code, I suppose sink is now an overloaded term. We should probably use the name observer.

@TylorS
Copy link
Member Author

TylorS commented Jan 11, 2016

I'm getting ready for sleep, so I'll have to try to get a better reply later, but personally I have no plans to ever use Rx. I plan to use Most only, and I don't want to have Rx in my code base if it's not required. That would be required with what you propose. My goals with this is to allow cycle-core to become a single place for both visions (and more) to exist. It's fair to say I'm probably on the outside looking in there.

It's also possible to wrap this up in other libraries with default adapters if that is desired.

npm install @cycle/rx @cycle/dom
npm install @cycle/most @cycle/dom
npm install @cycle/whatever @cycle/dom

At the end of the day, this isn't my library though.

@staltz
Copy link
Member

staltz commented Jan 11, 2016

I'm getting ready for sleep

Oh yeah, forgot the timezone issues!

@TylorS
Copy link
Member Author

TylorS commented Jan 11, 2016

Been up about 27 hours haha

@staltz
Copy link
Member

staltz commented Jan 11, 2016

personally I have no plans to ever use Rx. I plan to use Most only, and I don't want to have Rx in my code base if it's not required

Ok, that affects only this:

In fact, we can assume Rx as the default library

How bad would it be to publish packages

  • @cycle/core-rx
  • @cycle/core-most
  • @cycle/core-rx5
  • @cycle/core-bacon

To replace the {streamLibrary}-type API? Then we can freely determine that @cycle/core is an alias to @cycle/core-rx, and later on an alias for @cycle/core-rx5. It would mean writing Cycle Core over and over a few times, but it's small, and the end goal is to have a good API for users anyway. And in your case you would only have to use @cycle/core-most, no attachment to Rx.

@staltz
Copy link
Member

staltz commented Jan 11, 2016

Maybe that's what you meant with npm install @cycle/rx?

@staltz
Copy link
Member

staltz commented Jan 11, 2016

And again I'm caught thinking of bundling the stream library inside @cycle/core-the-stream-library if we are going to do the above API.

@TylorS
Copy link
Member Author

TylorS commented Jan 11, 2016

@cycle/rx

import Cycle from '@cycle/core'
import streamAdapter from 'cycle-rx-adapter'

const cycle = {
  run: (main, drivers) => Cycle.run(main, drivers, {streamAdapter})
}

export default cycle

@Cmdv
Copy link
Contributor

Cmdv commented Jan 11, 2016

this does seem the most intuitive to me npm install @cycle/rx 👍
Got to add really liking how this idea has bought everything back to a single point. Not a single atom of course :P

@staltz
Copy link
Member

staltz commented Jan 11, 2016

In that case we could rename core to base, since it's not meant to be directly used by users. As for the naming, I can see people thinking that @cycle/rx is a bundle of Rx library. We could name it instead @cycle/rx-run. "Run() for RxJS"

@TylorS
Copy link
Member Author

TylorS commented Jan 11, 2016

Should 'officially' supported adapters exist within @cycle/ as well?
Likely just rx4 rx5 and most

@TylorS
Copy link
Member Author

TylorS commented Jan 11, 2016

Other than that, I find naming to be trivial.

@staltz
Copy link
Member

staltz commented Jan 11, 2016

Yeah I think @cycle/most-adapter makes sense

@TylorS
Copy link
Member Author

TylorS commented Jan 11, 2016

If we rename @cycle/core to @cycle/base
Should we then create @cycle/core as an 'alias' to Rx as default? In theory 0 changes would be required by the end user.

@staltz
Copy link
Member

staltz commented Jan 11, 2016

Good point. We can do the alias @cycle/core => @cycle/rx-run but socially deprecate @cycle/core, encouraging people to use @cycle/rx-run (just the package name is different). Meaning that core is an older major version thingy. Idea is to not break compatibility.

@TylorS
Copy link
Member Author

TylorS commented Jan 11, 2016

That sounds appropriate. 👍

@staltz
Copy link
Member

staltz commented Jan 11, 2016

We can also alias @cycle/TylorS => @cycle/sleep

@TylorS
Copy link
Member Author

TylorS commented Jan 11, 2016

I'm inclined to suggest @cycle/TylorS to be the greatest package name of all time. No bias at all.

Loose list of things to do in no particular order

  • @cycle/core -> @cycle/base
  • Write test for @cycle/base ensuring one-liner drivers work properly with default adapter.
  • @cycle/core = @cycle/rx-run = @cycle/base + @cycle/rx-adapter
  • deprecate @cycle/core
  • Update all cycle drivers to use driverFn.streamAdapter = rxAdapter
  • Write Most.js adapter
  • create @cycle/most-adapter
  • create @cycle/most-run
  • Update motorcycle drivers to use driverFn.streamAdapter = mostAdapter

Anything else?

Should we move motorcycle-dom to @cycle/snabbdom or similar once it's up to date with the recent @cycle/dom changes regarding event delegation? Since that has a large part of what I/we have been working towards.

@staltz
Copy link
Member

staltz commented Jan 11, 2016

Everything correct.

And one thing just to be sure: add test on @cycle/base to make sure one-liner drivers use the same stream library as the run() is using, by default.

@TylorS
Copy link
Member Author

TylorS commented Jan 11, 2016

Updated

Injects adapt function for drivers with complex APIS that
need to do manual stream adaptation. e.g. DOM driver
@TylorS
Copy link
Member Author

TylorS commented Jan 11, 2016

})
return sinks
}
const adapt = stream => // eslint-disable-line
Copy link
Member

Choose a reason for hiding this comment

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

const streamAdaptation = ...

@staltz
Copy link
Member

staltz commented Jan 11, 2016

@TylorS I got your list of things to do, ordered them in a particular order, and put it in #196, since that's a better place for a to do list.

@Frikki
Copy link
Member

Frikki commented Jan 11, 2016

Because "just using Cycle with Rx" is going to be far more common than "I want to use Cycle with my own non-officially-supported streams library", an API like ... would be better. In fact, we can assume Rx as the default library.

By same logic, @cycle/dom should also then be default, because using it is going to be far more common than "I want to use Cycle with my own non-officially-supported DOM library".

The problem I see with Rx as default is that Rx will be included in my app code whether I use it or not.

In my view, if you want to provide users with a default setup, just make a new repo/package that has the necessary dependencies and imports to your liking. This will leave other repo/packages separated with their respective concerns. (I hear the crowd cheering for separation of concerns and the flexibility it provides).

@staltz
Copy link
Member

staltz commented Jan 11, 2016

@Frikki that comment was "deprecated". Remove the last sentence:

 Because "just using Cycle with Rx" is going to be far more common 
 than "I want to use Cycle with my own non-officially-supported streams 
 library", an API like ... would be better. 
-In fact, we can assume Rx as the default library.

@Frikki
Copy link
Member

Frikki commented Jan 11, 2016

@staltz Okay. Is the Rx dependency solved then? And how? I find it a bit difficult to follow the above discussion.

UPDATE
I think I got it. There’ll be cycle/base. And then cycle/core will be cycle/base + cycle/rx-adapter. Just as I wished. Thus, cycle/base will not have an Rx dependency, correct?

@staltz
Copy link
Member

staltz commented Jan 11, 2016

It is solved... I updated issue #196 summarizing our solution.

@TylorS
Copy link
Member Author

TylorS commented Jan 13, 2016

Closing this as we're moving this PR to base

@TylorS TylorS closed this Jan 13, 2016
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.

4 participants