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

Cycle Unified update #185

Closed
ntilwalli opened this issue Mar 12, 2017 · 6 comments
Closed

Cycle Unified update #185

ntilwalli opened this issue Mar 12, 2017 · 6 comments

Comments

@ntilwalli
Copy link
Collaborator

ntilwalli commented Mar 12, 2017

I have a fork which updates to Unified. It requires a PR (cyclejs/cyclejs#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
Copy link
Member

@stevenmathews

@stevenmathews
Copy link

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
Copy link
Collaborator Author

@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
Copy link
Member

TylorS commented Mar 13, 2017

Another option would be not to depend on the history driver

@ntilwalli
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

Released via #186

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

No branches or pull requests

4 participants