Rewrite middlewares to sagas #117

mik639 · 2018-07-29T13:47:41Z

Related Issue:
As was discussed in #97 I have rewrite custom middlewares with sagas, last one task.middleware should be rewritten by @superhawk610 (as he asked)

Summary:

Screenshots/GIFs:

joshwcomeau

Hi @mik639, thanks so much for this :D

I have a concern about how it'll merge with #103 - both PRs add a different dependency.saga.js. @superhawk610's PR is based around having a queue system to group dependency installations, and so I think we can probably use that version of the file, with any changes that @mik639 (as our local Redux Saga expert) suggests?

A misc note: Could you delete import-project.middleware.js if it's no longer being used?

For the most part this code looks great, but I had a few requested tweaks. Please let me know if you have any questions, or if there's anything I can do to facilitate you and @superhawk610 working together to get both your changes merged

joshwcomeau · 2018-07-30T12:45:00Z

src/sagas/import-project.saga.js

@@ -0,0 +1,160 @@
+import { call, put, cancel, select, takeEvery } from 'redux-saga/effects';


Would it be possible to add Flow typing to this file?

I'm not as worried about dependency.saga.js for two reasons:

The original dependency.middleware.js didn't have Flow typing, which was my bad

@superhawk610's Queue/group actions that lock package.json #103 PR adds Flow typing for this file.

If you're not comfortable/familiar with Flow, or if you just don't have the time to spare right now, feel free to just add a // TODO: Add Flow to the top of the file, and I or someone else will try to tackle it shortly after merging.

Sure, added

joshwcomeau · 2018-07-30T12:46:34Z

src/sagas/import-project.saga.js

+    // In the future, maybe I can attach a suffix like `-copy`, but for
+    // now I'll just reject it outright.
+    const isAlredyExist = yield select(getInternalProjectById, projectId);
+    if (isAlredyExist) throw new Error('project-name-already-exists');


Nit: for consistency with the rest of the codebase, can we avoid 1-line conditionals?

if (isAlreadyExist) { throw new Error('project-name-already-exists'); }

Sure, maybe we can use eslint for preventing mistakes like this ;)

joshwcomeau · 2018-07-30T12:46:44Z

src/sagas/import-project.saga.js

+    // support... but for now, I'm prioritizing an A+ experience for
+    // supported project types.
+    const type = yield call(inferProjectType, json);
+    if (!type) throw new Error('unsupported-project-type');


(same comment here about 1-line conditionals)

joshwcomeau · 2018-07-30T12:48:25Z

src/sagas/import-project.saga.js

+  // Only a single path should be selected
+  const [path] = paths;
+  yield put(importExistingProjectStart(path));
+}


Good extracted function 👍

joshwcomeau · 2018-07-30T12:51:24Z

src/sagas/import-project.saga.test.js

+      const saga = handlePathInput(paths);
+      expect(saga.next().value).toEqual(
+        put(importExistingProjectStart('path/to/first'))
+      );


I'm not super familiar with saga testing - wouldn't we want to test that it's equal to a plain object?

const expectedValue = importExistingProjectStart('path/to/first'); // { type: IMPORT_EXISTING_PROJECT_START, path: 'path/to/first' } expect(saga.next().value).toEqual(expectedValue);

In other words, shouldn't we check importExistingProjectStart() instead of put(importExistingProjectStart())?

we need to check that saga will dispatch action(effect put), in other words, we expect

expect(saga.next().value).toEqual(put({ type: IMPORT_EXISTING_PROJECT_START, path: 'path/to/first' }));

@joshwcomeau that's one of the really cool features of redux-saga that makes testing so consistent - put(/*...*/) doesn't actually dispatch an action, it describes dispatching the action, so all you have to do when testing is check that the return from the saga matches the expected description.

It's a unique pattern but it has a handful of unique benefits - you can read more here.

ahhh, right. Awesome!

joshwcomeau · 2018-07-30T12:54:34Z

src/sagas/import-project.saga.test.js

+    });
+  });
+
+  test('showImportDialog saga', () => {


Could we use it instead of test everywhere?

Yes, we can, I didn't use it here, only because it looks not so semantic :)

it('runs showImportDialog saga correctly'?

I prefer to move it in its own describe section because it'll help to keep proper hierarchy in tests result

joshwcomeau · 2018-07-30T12:58:45Z

src/sagas/import-project.saga.test.js

+    });
+
+    it('should import project', () => {
+      global.Date = { now: () => 1532809641976 };


ah, right, the createdAt is done impurely within the saga, same as it was within the middleware.

This solution worries me, though (it won't be "unmocked" when this test file finishes, right? So this could affect subsequent tests). We could have an afterAll that restores it, but I wonder if there's a better way...

Maybe importProject could take a second optional argument, getCurrentTime, which could default to Date.now. In the test you could provide a mock function like () => 1532809641976.

What do you think?

You're right. I can replace it with:

const spyOnDate = jest.spyOn(Date, 'now'); spyOnDate.mockReturnValue(1532809641976); // test spyOnDate.mockRestore()

in this case mocked value will affect only one testcase

joshwcomeau · 2018-07-30T12:59:15Z

src/sagas/import-project.saga.test.js

+  });
+
+  describe('inferProjectType helper', () => {
+    it('should return null if field "dependencies" not exist in json', () => {


Thanks for checking this 👍

joshwcomeau · 2018-07-30T12:59:38Z

src/sagas/import-project.saga.test.js

+      expect(inferProjectType(json)).toBe('create-react-app');
+    });
+
+    it('should return "create-react-app" if json have dependency eslint-config-react-app', () => {


Thanks for checking this as well! Great coverage 👍

joshwcomeau · 2018-07-30T13:02:37Z

src/sagas/dependency.saga.js

+  yield takeEvery(ADD_DEPENDENCY_START, addDependency);
+  yield takeEvery(UPDATE_DEPENDENCY_START, updateDependency);
+  yield takeEvery(DELETE_DEPENDENCY_START, deleteDependency);
+}


Note: I didn't give this file a thorough review since I believe #103 implements the same thing but also takes the queue into account. Would be great to get your feedback on that approach, @mik639!

ok I'll take a look

mik639 · 2018-07-30T19:03:11Z

In the situation when we have a few pull requests with rewritten middlewares, I suggest to create a separate branch for merging and testing them, instead of merging to master. What do you think, @joshwcomeau ?

superhawk610 · 2018-07-31T00:23:52Z

@mik639 That's a good idea, you should create a sagas branch or similar on the main fork and we can consolidate our codebase there. As @joshwcomeau said, I have some existing work in #103 that I'll need to integrate with yours, as well as my tasks middleware implementation in #111 (feel free to review when you get a chance).

My main goal tonight is to get some ground work done on #77, but then I'll be turning my full attention to this no later than tomorrow.

Are you developing on Mac or Windows? I'm working from Windows so I have a few Windows-specific lines throughout, I figure that's fine since we'll be merging in Windows support to master sooner or later.

First order of business once I get some progress made on #77 is to review this.

joshwcomeau · 2018-08-01T13:57:57Z

@mik639 ++, like the idea of a working branch to merge and test everything 👍

mik639 · 2018-08-01T14:34:58Z

@superhawk610 @joshwcomeau I'm created a branch move-middleware-to-sagas and fixed comments for this pr. So let's define the order of merging #103 #111 and #117 😺

superhawk610 · 2018-08-01T15:16:49Z

@mik639 I think you should merge yours (#117) first, since it contains the bulk of the work, and then I will merge #103 and #111 in that order to make sure that they conform to your structure.

I haven't written tests for task.middleware but I will do that later today or tomorrow. I'll get my review for this PR up in a few hours.

joshwcomeau · 2018-08-01T20:04:57Z

Yay, so happy that y'all are working towards getting this stuff landed. I think we'll pen a release once this is in, since having the queue and a totally-reworked side effects layer seems like a good thing to capture in a launch :D In addition to all the great stuff folks have been merging in over the last week or two.

EDIT: Er, right, we'll also need the UI for the queue, which isn't covered in #103. I got a little ahead of myself 😅. Certainly when the UI is in, though!

superhawk610 · 2018-08-01T22:41:18Z

@mik639 I'm working on reviewing this, but now I'm running into the issue @joshwcomeau described here. I'm going to try switching it over to yarn temporarily to see if that fixes it. Darn Windows 😤

Alright, I've got a Windows version of this PR up at superhawk610/guppy if anybody else wants to follow along. Saga functionality is 💯.

superhawk610

Requesting changes for a minor spelling change. I'd also like to agree on a rule for consistency - yield call([object, 'method']) vs. yield call(object.method).

superhawk610 · 2018-08-01T22:59:25Z

src/sagas/dependency.saga.js

+    );
+    yield put(addDependencyFinish(projectId, dependency));
+  } catch (err) {
+    yield call([console, 'error'], 'Failed to install dependency', err);


I'm unfamiliar with redux-saga convention when using call with an object property. Why use

yield call([console, 'error'], /*...*/);

instead of

yield call(console.error, /*...*/);

? When implementing similar methods, I'm not sure which syntax to use (eg - ipcRenderer.send and childProcess.spawn).

superhawk610 · 2018-08-01T23:02:19Z

src/sagas/import-project.saga.js

+  switch (err.message) {
+    case 'project-name-already-exists': {
+      yield call(
+        dialog.showErrorBox,


Here's an example of what I mentioned in my previous comment - why use dialog.showErrorBox over [dialog, 'showErrorBox']?

superhawk610 · 2018-08-01T23:06:20Z

src/sagas/import-project.saga.js

+    // In the future, maybe I can attach a suffix like `-copy`, but for
+    // now I'll just reject it outright.
+    const isAlredyExist = yield select(getInternalProjectById, projectId);
+    if (isAlredyExist) {


Should be either isAlreadyExist or alreadyExists.

I vote for "alreadyExists", flows better IMO :)

superhawk610 · 2018-08-01T23:14:01Z

src/sagas/import-project.saga.test.js

+      const saga = handlePathInput(paths);
+      expect(saga.next().value).toEqual(
+        put(importExistingProjectStart('path/to/first'))
+      );


@joshwcomeau that's one of the really cool features of redux-saga that makes testing so consistent - put(/*...*/) doesn't actually dispatch an action, it describes dispatching the action, so all you have to do when testing is check that the return from the saga matches the expected description.

It's a unique pattern but it has a handful of unique benefits - you can read more here.

mik639 · 2018-08-02T11:05:16Z

@superhawk610 unfortunately flow-typed redux-saga currently have the issue with describing call effect, so for avoiding flow error, we need to use call([console, console.error], error) instead of call([console, 'error'], error) or adding ignore comment in places like this 😞

superhawk610 · 2018-08-02T15:25:20Z

So these three are functionally equivalent - here's a quick test:

const { call } = require('redux-saga/effects')

console.log(
  JSON.stringify(call([console, 'error'], 'foo')) + '\n' +
  JSON.stringify(call([console, console.error], 'foo')) + '\n' +
  JSON.stringify(call(console.error, 'foo'))
)

// {"@@redux-saga/IO":true,"CALL":{"context":{},"args":["foo"]}}
// {"@@redux-saga/IO":true,"CALL":{"context":{},"args":["foo"]}}
// {"@@redux-saga/IO":true,"CALL":{"context":null,"args":["foo"]}}

@mik639 None of these patterns throw an error with Flow for me

yield call([console, 'error'], 'Failed to install dependency', err);
yield call([console, console.error], 'Failed to install dependency', err);
yield call(console.error, 'Failed to install dependency', err);

flow version: 0.73.0
redux-saga version: 0.16.0

mik639 · 2018-08-02T18:22:56Z

@superhawk610 first two - yes, there are same, but third as you can see has no context.

Redux-saga itself has no flow types, it should be typed with libdef.
In my pr I have added libdef for saga from https://github.com/flow-typed/flow-typed

superhawk610 · 2018-08-02T18:57:16Z

Ah, that makes more sense. I didn't have those flow-type definitions pulled on my work machine, just at home, so I didn't catch it.

Do you want to switch all of the call() functions to use call(object, object.function) syntax then?

mik639 · 2018-08-06T11:28:57Z

@superhawk610 @joshwcomeau Sorry for delay, I have fixed comments, and have updated pr

superhawk610 · 2018-08-06T15:13:33Z

@mik639 Very nice fix with destructuring electron.remote.dialog. @joshwcomeau I'm going to go ahead and merge this into the feature branch and open a separate PR later this afternoon with my changes, if you have any further comments just leave them there and we will address them.

joshwcomeau · 2018-08-09T19:34:51Z

Thanks for merging @superhawk610! Took a quick glance, looks great to me!

* Replace middlewares with sagas * Remove import-project middleeare from prod store configuration * Add flow-typed redux-saga, review fixes * review fixes

* Rewrite middlewares to sagas (#117) * Tasks middleware to saga (#122) * pass test suite - fix getInternalProjectById in projects.reducer test - move projects.reducer test up a directory - add missing assertions in create-project.service test - remove middlewares directory * review tweaks * bring in line with existing sagas - add note about refactoring stderr handling * update test to use new name (isDevServerFail) * Make some small tweaks before merging * Fix ESLint errors

mik639 added 3 commits July 29, 2018 17:22

Replace middlewares with sagas

ff38003

Merge branch 'master' into rewrite-middlewares-to-sagas

46c551f

Remove import-project middleeare from prod store configuration

3514f1c

mik639 requested review from joshwcomeau and superhawk610 July 29, 2018 13:48

joshwcomeau suggested changes Jul 30, 2018

View reviewed changes

Add flow-typed redux-saga, review fixes

8af13ec

joshwcomeau mentioned this pull request Aug 1, 2018

Project configuration #28

Closed

mik639 changed the base branch from master to move-middleware-to-sagas August 1, 2018 14:12

superhawk610 suggested changes Aug 1, 2018

View reviewed changes

review fixes

0624212

superhawk610 approved these changes Aug 6, 2018

View reviewed changes

superhawk610 merged commit a03b326 into joshwcomeau:move-middleware-to-sagas Aug 6, 2018

superhawk610 mentioned this pull request Aug 7, 2018

Tasks middleware to saga #122

Merged

superhawk610 pushed a commit that referenced this pull request Aug 18, 2018

Rewrite middlewares to sagas (#117)

b072a9b

* Replace middlewares with sagas * Remove import-project middleeare from prod store configuration * Add flow-typed redux-saga, review fixes * review fixes

joshwcomeau mentioned this pull request Aug 20, 2018

Redux saga for easy testing and extending #97

Closed

superhawk610 pushed a commit that referenced this pull request Aug 23, 2018

Rewrite middlewares to sagas (#117)

ee106f7

* Replace middlewares with sagas * Remove import-project middleeare from prod store configuration * Add flow-typed redux-saga, review fixes * review fixes

superhawk610 mentioned this pull request Aug 23, 2018

Move middlewares to sagas #154

Merged

superhawk610 pushed a commit that referenced this pull request Aug 30, 2018

Rewrite middlewares to sagas (#117)

10c0b0d

		@@ -0,0 +1,160 @@
		import { call, put, cancel, select, takeEvery } from 'redux-saga/effects';

Rewrite middlewares to sagas #117

Rewrite middlewares to sagas #117

Conversation

mik639 commented Jul 29, 2018

joshwcomeau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik639 Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik639 commented Jul 30, 2018

superhawk610 commented Jul 31, 2018

joshwcomeau commented Aug 1, 2018

mik639 commented Aug 1, 2018

superhawk610 commented Aug 1, 2018

joshwcomeau commented Aug 1, 2018 • edited Loading

superhawk610 commented Aug 1, 2018 • edited Loading

superhawk610 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik639 commented Aug 2, 2018

superhawk610 commented Aug 2, 2018

mik639 commented Aug 2, 2018

superhawk610 commented Aug 2, 2018

mik639 commented Aug 6, 2018

superhawk610 commented Aug 6, 2018

joshwcomeau commented Aug 9, 2018

mik639 Jul 30, 2018 •

edited

Loading

joshwcomeau commented Aug 1, 2018 •

edited

Loading

superhawk610 commented Aug 1, 2018 •

edited

Loading