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

Upgrade deps #26

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Upgrade deps #26

wants to merge 9 commits into from

Conversation

iainkirkpatrick
Copy link
Member

@iainkirkpatrick iainkirkpatrick commented Jul 3, 2018

  • upgrades the lib dependencies to latest stable versions
  • fixes tests (this needs assessment)

major changes:

  • rxjs v6 syntax + redux-observable v1 API

@iainkirkpatrick iainkirkpatrick changed the title WIP: Upgrade deps Upgrade deps Jul 7, 2018
@iainkirkpatrick
Copy link
Member Author

@ahdinosaur if you have time in the next wee while i'd super appreciate your eye on this - i feel i still have a less-than-ideal overall picture of feathers-action, in particular i've fixed the tests to use what seems to me to be the correct latest rxjs / redux-observable approaches, it'd be rad to have your input on that though.

In particular, the final return of createEpics in epic.js - i think this is right, and it seems to work with dogstack-example, it looks like perhaps there is some current trouble with takeUntil (ReactiveX/rxjs#3853) which is why i've changed to the takeWhile approach.

Copy link
Member

@ahdinosaur ahdinosaur left a comment

Choose a reason for hiding this comment

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

most everything looks pretty gravy! 🥔

the only part is where you had a question:

In particular, the final return of createEpics in epic.js - i think this is right, and it seems to work with dogstack-example, it looks like perhaps there is some current trouble with takeUntil (ReactiveX/rxjs#3853) which is why i've changed to the takeWhile approach.

my only concern is that as far as i understand, you removed the ability to cancel the subscription, which is kinda fundamental. for example, every get and find request created by feathers-action-react is created when a component mounts and cancelled when a component unmounts. i guess there isn't a test for that?

i think what you have is good, you could probably just change isEndAction is include the cancel action.

concat(of(actionCreators.complete(cid))),
catchError(err => of(actionCreators.error(cid, err))),
takeWhile(endOnce(isEndAction))
// takeUntil(cancelAction$),
Copy link
Member

Choose a reason for hiding this comment

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

how do you cancel then?

Copy link
Member Author

Choose a reason for hiding this comment

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

have just gotten around to looking at this now :| hmm, correct me if i'm wrong 🙏 but isn't a cancel action just a 'complete' action? from what i see in feathers-action-react? the function isEndAction there is an either of complete or error actions, so my understanding is that takeWhile will take until endOnce is false, which happens when an 'endAction' is passed in. Though now I look at endOnce more closely, it looks like it will wait one more value of the stream until doing so... hmmm

as far as tests go, test 'find with cancel' should be testing this behaviour i think?

test('find with cancel', function (t) {

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.

2 participants