search

cyclejs-community / cyclic-router

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

Cycle Unified update #185

Closed ntilwalli closed 7 years ago

ntilwalli commented 7 years ago

I have a fork which updates to Unified. It requires a PR (https://github.com/cyclejs/cyclejs/pull/552) to be merged into cyclejs/history, but other than that it works. I'll post the PR when (assuming) that history PR gets merged. You can view/test the changes here: https://github.com/ntilwalli/cyclic-router

The above linked repo includes a cyclejs submodule that needs to be pulled in and switched to the cyclic-history branch for the 'npm install' to succeed.

The update introduces a breaking change in that the create function no longer takes options. You'll have to instantiate your history object externally (where you can apply your options).

Reviews/critiques are welcome.

Widdershin commented 7 years ago

@stevenmathews

stevenmathews commented 7 years ago

Hi, I was also working on updating this to Unified but hadn't opened an issue first. Lesson learned :-)

Main difference is that I was requiring people to pass in a history driver but ran into problems when I didn't have access to the createHref method.

ntilwalli commented 7 years ago

@stevenmathews adding a history driver which allows injecting the history object externally is exactly the PR I've submitted to @cycle/history. See the linked PR in initial comment.

TylorS commented 7 years ago

Another option would be not to depend on the history driver

ntilwalli commented 7 years ago

@TylorS did you see the PR I submitted? It's a trivial change/addition to @cyclejs/history and made updating this driver relatively simple. I'd have to think about your suggestion but it sounds like that would mean replicating the logic in the history driver over here. Is that correct? Assuming the above PR gets merged, I'd lean towards keeping the current architecture since it's more compositional...

ntilwalli commented 7 years ago

Released via https://github.com/cyclejs-community/cyclic-router/pull/186