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

Compatible with redux-saga 1.0.0 #242

Merged
merged 11 commits into from Feb 5, 2019

Conversation

Projects
None yet
9 participants
@jp928
Copy link
Collaborator

jp928 commented Jan 25, 2019

No description provided.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 25, 2019

Codecov Report

Merging #242 into master will decrease coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #242      +/-   ##
==========================================
- Coverage     100%   99.85%   -0.15%     
==========================================
  Files          22       22              
  Lines         715      704      -11     
  Branches      143      140       -3     
==========================================
- Hits          715      703      -12     
- Misses          0        1       +1
Impacted Files Coverage Δ
src/expectSaga/parseEffect.js 100% <ø> (ø) ⬆️
src/expectSaga/provideValue.js 100% <ø> (ø) ⬆️
src/testSaga/index.js 100% <ø> (ø) ⬆️
src/testSaga/validateEffects.js 100% <100%> (ø) ⬆️
src/expectSaga/index.js 100% <100%> (ø) ⬆️
src/shared/serializeEffect.js 90% <0%> (-10%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f78a07...160d1a7. Read the comment docs.

@@ -0,0 +1,31 @@
{

This comment has been minimized.

@VincentLanglet

VincentLanglet Jan 26, 2019

IDE settings should not be in the pr

This comment has been minimized.

@jp928

jp928 Jan 27, 2019

Author Collaborator

@VincentLanglet
Thanks for your review. I will revise them.

fn === takeLeadingHelper
);
}
// function isHelper(fn: Function): boolean {

This comment has been minimized.

@VincentLanglet

VincentLanglet Jan 26, 2019

Why not removing all the commented code

@@ -44,7 +41,8 @@ import {
TAKE,
} from '../shared/keys';

const { asEffect, is } = utils;
// const { asEffect, is } = utils;

This comment has been minimized.

@VincentLanglet

VincentLanglet Jan 27, 2019

You forgot here

This comment has been minimized.

@jp928

jp928 Jan 27, 2019

Author Collaborator

My bad. Will revise.

@@ -560,7 +536,8 @@ export default function expectSaga(
api.put.like = createEffectTester(
'put',
PUT,
effects.put,
// effects.put,

This comment has been minimized.

@VincentLanglet
import { effects, eventChannel } from 'redux-saga';
import * as effects from 'redux-saga/effects';

// import { eventChannel } from 'redux-saga';

This comment has been minimized.

@VincentLanglet
@VincentLanglet

This comment has been minimized.

Copy link

VincentLanglet commented Jan 27, 2019

@jfairbank This PR is really needed. Thanks if you can look at

@feugy

This comment has been minimized.

Copy link

feugy commented Jan 28, 2019

Thanks a million @VincentLanglet and @jfairbank!

@jfairbank, until this is published on NPM, would you mind building the sources and committing them in your repository?
Si we could use it as dependency in package.json files: "redux-saga-test-plan": "jp928/redux-saga-test-plan"

@jp928

This comment has been minimized.

Copy link
Collaborator Author

jp928 commented Jan 28, 2019

Thanks a million @VincentLanglet and @jfairbank!

@jfairbank, until this is published on NPM, would you mind building the sources and committing them in your repository?
Si we could use it as dependency in package.json files: "redux-saga-test-plan": "jp928/redux-saga-test-plan"

You can use git link in your package.json. It is little bit ugly but before this is merged it would be a solution for time being.

@GioSensation

This comment has been minimized.

Copy link

GioSensation commented Jan 29, 2019

@jp928 Thanks for your work on this. I hope it is merged soon. But in the meantime, could you please explain in more details how to include your version in my current project?

I have this in my package.json:

"redux-saga-test-plan": "git+https://github.com/jfairbank/redux-saga-test-plan.git#be0e52bfcf0b08346d52b42bd753929e1c61d06e",

This of course got me your latest changes, but when I try running the tests, I get Cannot find module 'redux-saga-test-plan'.

@feugy

This comment has been minimized.

Copy link

feugy commented Jan 29, 2019

@GioSensation you have to build react-saga-test-plan by yourself.

cd node_modules/react-sata-test-plan
npm install
npm run build

That's why I've asked to include the built files in the repo, to avoid this manual step...

@davidjbradshaw

This comment has been minimized.

Copy link

davidjbradshaw commented Jan 29, 2019

@jp928 @VincentLanglet @feugy Looks like this project is looking for a new maintainer
https://twitter.com/elpapapollo/status/1069693818985795584

@fttr

This comment has been minimized.

Copy link

fttr commented Jan 29, 2019

@GioSensation I think you have to use the fork from @jp928 instead of @jfairbank:

"redux-saga-test-plan": "git+https://github.com/jp928/redux-saga-test-plan.git#be0e52bfcf0b08346d52b42bd753929e1c61d06e",

@GioSensation

This comment has been minimized.

Copy link

GioSensation commented Jan 29, 2019

@fttr The hash is the same, so it actually works. You still have to npm run build manually, though.

After this and a few tweaks here and there I got most tests working. The only failing tests are the ones that try to assert the use of delay. In the new redux-saga version, it is now an effect and it has been moved in redux-saga/effects. Anybody else experiencing this? I get TypeError: jasmine.Spec.isPendingSpecException is not a function. I couldn't figure it out as yet.

@jfairbank

This comment has been minimized.

Copy link
Owner

jfairbank commented Jan 29, 2019

Hi, @jp928. Thanks so much for doing this and offering to maintain! Sorry to everyone that has been waiting on this and other fixes. Life has made open source time very difficult for me.

@jp928 Would you mind updating the CHANGELOG with any important changes before we merge?

@jp928

This comment has been minimized.

Copy link
Collaborator Author

jp928 commented Jan 29, 2019

Hi, @jp928. Thanks so much for doing this and offering to maintain! Sorry to everyone that has been waiting on this and other fixes. Life has made open source time very difficult for me.

@jp928 Would you mind updating the CHANGELOG with any important changes before we merge?

Hi, @jfairbank
Thanks for your reply. The APIs not change at all but I will update CHANGELOG anyways.
I may start to make pull requests based on the issue lists in the future. Also, I would like to have your guidance about your idea how to upgrade this library. Thanks for inventing this handy library I like it so much.

@@ -1,8 +1,9 @@
## v4.0.0-beta.1

This comment has been minimized.

@davidjbradshaw

davidjbradshaw Jan 30, 2019

Won't this be v4.0.0-beta.2 as 1 is already published

This comment has been minimized.

@jp928

jp928 Jan 30, 2019

Author Collaborator

Sure. Updated.

This comment has been minimized.

@davidjbradshaw

davidjbradshaw Jan 31, 2019

That should most likely be a new entry, with just with what is in your PR listed. Rather than editing beta 1 release notes

This comment has been minimized.

@jp928

jp928 Jan 31, 2019

Author Collaborator

@davidjbradshaw
Gotcha.

jp928 added some commits Jan 30, 2019

@davidjbradshaw

This comment has been minimized.

Copy link

davidjbradshaw commented Feb 1, 2019

@jfairbank this looks good to go now

@@ -1,6 +1,6 @@
{
"name": "redux-saga-test-plan",
"version": "4.0.0-beta.2",
"version": "4.0.0-beta.3",

This comment has been minimized.

@davidjbradshaw

davidjbradshaw Feb 4, 2019

Did 4.0.0-beta.2 get pushed to npm? As I don't see it.

This comment has been minimized.

@jp928

jp928 Feb 4, 2019

Author Collaborator

Nope. So we should stay on beta.2?

@jfairbank jfairbank merged commit 6089d83 into jfairbank:master Feb 5, 2019

3 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project Absolute coverage decreased by -0.14% but relative coverage remained the same compared to 7f78a07
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jfairbank

This comment has been minimized.

Copy link
Owner

jfairbank commented Feb 5, 2019

Published to npm under the beta tag! Thanks @jp928 for the work and others for the feedback!

@chasecaleb

This comment has been minimized.

Copy link

chasecaleb commented Feb 5, 2019

The TypeScript type definitions are still broken because of a couple missing generics.

[my-project]/node_modules/redux-saga-test-plan/effects.d.ts
Type error: Generic type 'Pattern' requires 1 type argument(s).  TS2314

    11 | 
    12 | interface TakeEffectApi<R> {
  > 13 |     (pattern?: E.Pattern): R;
       |                ^
    14 |     <T>(channel: Channel<T>): R;
    15 | }
    16 | interface PutEffectApi<R> {

Same issue with line 13 of the same file:

interface TakeEffectApi<R> {
    (pattern?: E.Pattern): R; // This line
    <T>(channel: Channel<T>): R;
}

The definition of Pattern from redux-saga is in @redux-saga/types/index.d.ts on line 24:

export type Pattern<T> = SubPattern<T> | SubPattern<T>[]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.