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

fp-ts@2 support added #34

Merged
merged 5 commits into from
Aug 13, 2019
Merged

fp-ts@2 support added #34

merged 5 commits into from
Aug 13, 2019

Conversation

mlegenhausen
Copy link
Contributor

@mlegenhausen mlegenhausen commented Jun 26, 2019

I added the minimum changeset to get the library working with fp-ts@2. I was not able to make it backwards compatible cause of the removal of Monodial, the changes in the Foldable and Alternative instances.

Would be great if you could take a first look and give me feedback.

I add a fp-ts-v2-lib branch so you can try it out directly

yarn add https://github.com/werk85/remote-data-ts.git#fp-ts-v2-lib

export interface RemoteDataT<F> extends ApplicativeComposition<F, URI> {
export interface RemoteDataT<M, E, A> extends HKT<M, RemoteData<E, A>> {}

export interface RemoteDataM<F> extends ApplicativeCompositionHKT2<F, URI> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YBogomolov Could you help with reviewing the transformer module please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I like how the code style is streamlined with fp-ts@2 style 👍🏻

src/remote-data.ts Outdated Show resolved Hide resolved
src/remote-data.ts Outdated Show resolved Hide resolved
@raveclassic
Copy link
Contributor

@mlegenhausen Thanks for you work! However we can't merge this to master because of direct repo reference in package.json. In spite of that we can release an .rc version from this branch if you need it.

/cc @sutarmin

@raveclassic
Copy link
Contributor

#33

export interface RemoteDataT<F> extends ApplicativeComposition<F, URI> {
export interface RemoteDataT<M, E, A> extends HKT<M, RemoteData<E, A>> {}

export interface RemoteDataM<F> extends ApplicativeCompositionHKT2<F, URI> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I like how the code style is streamlined with fp-ts@2 style 👍🏻

@mlegenhausen
Copy link
Contributor Author

Before we merge I would like to add the pipeable operators and deprecate the class operators? This would allow us in a next step to go classless without the further introduction of breaking changes.

@mlegenhausen
Copy link
Contributor Author

I reworked the code to add pipeable operators and changed the test accordantly. I also added the getOrElse, fold, toUndefined and toNullable operators. Because adding operators introduce no breaking changes I only added the one that are most useful for my use cases.

@raveclassic
Copy link
Contributor

@mlegenhausen Thanks for you work!

@sutarmin I'd like to discuss if we're ready to go classless. Migration to [email protected] is already a breaking change, is there any reason we'd want to split migration steps?

@sutarmin
Copy link

sutarmin commented Jul 2, 2019

I don't see any reasons for separating fp-ts@2 support and classless approach for rd itself. I think even quite the opposite, it would make much more sense for rd consumers to migrate both fp-ts and rd at ones, so in my opinion, we need to provide fp-ts@2-way API as fast as possible.

But regarding this PR, we can either rewrite to classless approach here or merge this one (once io-ts-types has a version with fp-ts@2 support) and continue working it separate PR. Again, to provide users new API faster, I would go with former.

@raveclassic
Copy link
Contributor

So, summing up, we're still waiting for io-ts-types release. In the meantime we could drop classes

@YBogomolov
Copy link
Contributor

@raveclassic, just FYI – [email protected] was released with support for fp-ts@2: https://github.com/gcanti/io-ts-types/releases/tag/0.5.0

@sutarmin
Copy link

Nice! Now I think we could update this PR, merge it and then work on making RD classless is separate PR, to be more granular.
What do you guys think?

/cc @raveclassic @mlegenhausen

@raveclassic
Copy link
Contributor

Agree on moving classless to separate PR

@mlegenhausen
Copy link
Contributor Author

I updated the pull request.

Would be ok from my side. Could we then already start with a next version package release for npm?

@raveclassic raveclassic merged commit d51fcd5 into devexperts:master Aug 13, 2019
raveclassic pushed a commit that referenced this pull request Aug 13, 2019
* fp-ts@2 support added
* Code cleanups
* Comment 2v suffix removed
* pipeable operators added

BREAKING CHANGE: fp-ts and io-ts-types dependencies updated to latest stable version
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

Successfully merging this pull request may close these issues.

4 participants