search

cyclejs-community / cyclic-router

Router Driver built for Cycle.js
MIT License
109 stars 25 forks source link

Change router to main wrapper #200

Closed jvanbruegge closed 6 years ago

jvanbruegge commented 6 years ago

This will allow for easier testing, because the router is only a pure wrapper for the main function. You can simply add it in your tests and dont have to mock it.

I'm currently converting the tests. What do you think @ntilwalli?

jvanbruegge commented 6 years ago

This would be done from my side, of course this is a major breaking change, so it would be v5.0.0

jvanbruegge commented 6 years ago

cc @staltz

ntilwalli commented 6 years ago

For a new major version, I think this sounds great!

staltz commented 6 years ago

Looks good! I've also been thinking a lot about routing (for instance I was considering how to wrap React Navigation for Cycle.js apps in React Native) and came to conclusion it's logic, not effects. I haven't reviewed this PR carefully, but I saw the shift from makeDriver to routerify.

jvanbruegge commented 6 years ago

:)

jvanbruegge commented 6 years ago

So, I've removed unneeded dependencies from the package.json, everything should be good to go

ntilwalli commented 6 years ago

@jvanbruegge I added you as a collaborator on github and npm

jvanbruegge commented 6 years ago

I already wondered about the automatic subscription :)

jvanbruegge commented 6 years ago

Should we merge this?

ntilwalli commented 6 years ago

I posted a question above, but I'm fine with merging

jvanbruegge commented 6 years ago

Added the option

jvanbruegge commented 6 years ago

Anything left to do?

ntilwalli commented 6 years ago

Looks good to me. I'll leave the merging and publishing to you.

ntilwalli commented 6 years ago

If history is omitted from sources shouldn't the key be deleted from sources instead of just set to undefined? As it stands, Object.keys(sources) will still show history and that's odd...

jvanbruegge commented 6 years ago

I can do that