cyclejs / history

MOVED
59 stars 8 forks source link

Update to history 3.0.0 #23

Open TylorS opened 8 years ago

TylorS commented 8 years ago

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 commented 8 years ago

I can take a look at it ;)

michalvankodev commented 8 years ago

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 commented 8 years ago

(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 commented 8 years ago

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 commented 8 years ago

It can be dealt with drop(1)

True

michalvankodev commented 8 years ago

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

michalvankodev commented 8 years ago

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 commented 8 years ago

Added some comments :)

wpcfan commented 8 years ago

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 commented 8 years ago

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

nicoespeon commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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.