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

Update to history 3.0.0 #23

Open
TylorS opened this issue Apr 30, 2016 · 14 comments
Open

Update to history 3.0.0 #23

TylorS opened this issue Apr 30, 2016 · 14 comments

Comments

@TylorS
Copy link
Member

TylorS commented Apr 30, 2016

History 3.0.0 has some breaking changes, need to more closely investigate what they are and how they affect things.

With v3.0.0 it now also provided a dist/ build via npmcdn, we should also now create dist/ build of @cycle/history.

@michalvankodev
Copy link
Contributor

I can take a look at it ;)

@michalvankodev
Copy link
Contributor

Well The list of breaking changes is really small: https://github.com/mjackson/history/blob/master/CHANGES.md

I took a look and many of those doesn't affect anything.
I ran the tests and all passed. (Chrome didn't close the window after the tests.)
I think only one thing that can be breaking is

Breakage: history.listen no longer calls the callback synchronously once. Use history.getCurrentLocation instead

But it seems like this is wanted behaviour.

@TylorS
Copy link
Member Author

TylorS commented May 3, 2016

(Chrome didn't close the window after the tests.)

Yeah that's normal - its so you can debug issues. testem ci runs them an closes automatically for running on travis ci.

Breakage: history.listen no longer calls the callback synchronously once. Use history.getCurrentLocation instead

But it seems like this is wanted behaviour.

Hmmm, that makes me wonder if there should be tests for the initial 'POP' action. I think is actually wanted behavior client-side (without doing server rendering), as it gives you the initial location. Perhaps it could be an optional flag.

@michalvankodev
Copy link
Contributor

michalvankodev commented May 5, 2016

That's true. If you have only client side app and you want to move to some route you will actually need to emit that action.

Perhaps it could be an optional flag.

It doesn't have to be a flag. It can be dealt with .drop(1) on the client render as with DOM.

@TylorS
Copy link
Member Author

TylorS commented May 5, 2016

It can be dealt with drop(1)

True

@michalvankodev
Copy link
Contributor

I'll write a test for initial value and we will see if it fails with history 3

@michalvankodev
Copy link
Contributor

#24 :)

I'm still thinking about removing ability to push items into ServerHistory. And if it would remove the hassle of completing the stream without calling .complete().

@TylorS
Copy link
Member Author

TylorS commented May 5, 2016

Added some comments :)

@wpcfan
Copy link

wpcfan commented Aug 5, 2016

seems still not working with typescript, and I checked the ./src/interfaces.ts, the type createLocation(location: Location | Pathname): Location is not compatible with history 3.0

@wpcfan
Copy link

wpcfan commented Aug 5, 2016

Looks like the types defined in interfaces.ts are not compatible with the type definition file ( typings install npm~history --global).

@nicoespeon
Copy link

Hello there!

Yup, I confirm @wpcfan's statement about interfaces.ts. I was struggling with using cyclic-router in a TS project. After digging it looks like there is typings incompatibility.

Trying to use cyclic-router v2.1.2 and history v3.0.0 fails because makeRouterDriver(createHistory()) triggers a compatibilty error.

Indeed, History#listen signature (listen(listener: Listener): () => void;) is not compatible with the history d.ts from npm (listen(listener: LocationListener): Function;).

Is there something I could do to help?

Thanks!

@TylorS
Copy link
Member Author

TylorS commented Aug 17, 2016

Hello! Yeah, I'm pretty aware of the issues, unfortunately this is the first TS project I've ever done (Many more since). I have not been given the opportunity to fix them.

I think it's possible, and likely better to generalize the types used in this library so that history works out of the box. I didnt, and still don't want to directly depend upon the history library.

PRs and advice are super welcome

@nicoespeon
Copy link

I'm quite new to TS but that would be a great exercise. Plus, I'm stuck and can't achieve to set up a working router with Cycle.js Diversity & TS because of this − or I'm missing something.

I'll give it a look and see what I can do, try to propose something 😄

Thanks 👍

@TylorS
Copy link
Member Author

TylorS commented Aug 17, 2016

I'll be using this library and my other poorly typed concoction cyclic-router in a production environment soon, with TypeScript, so fixes are "imminent" if no one else gets to this before me.

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