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.

Sign up for GitHub

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

Jump to bottom

feat(cmf): add redux-saga integration at component level #1055

Merged
merged 32 commits into from
Feb 16, 2018
Merged

feat(cmf): add redux-saga integration at component level #1055

merged 32 commits into from
Feb 16, 2018

Conversation

jmfrancois
Copy link
Contributor

@jmfrancois jmfrancois commented Feb 8, 2018 •
edited
Loading

What is the problem this PR is trying to solve?

Today we have the saga router, so you can start / stop saga on each route.
You can't do that at component level

What is the chosen solution to this problem?

If your component is cmfConnected you can pass a props.saga (string) which will be start & stop by the saga api.sagas.component.handle that you should register in your saga middleware.

to support the saga as a string cmf pattern you have the following api to use in your configure.js

import { api } from '@talend/react-cmf';
import bootstrap from './sagas/bootstrap';

api.sagas.register('bootstrap', bootstrap);

Please check if the PR fulfills these requirements

  • The PR commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Related design / discussions / pages (not in jira), if any, are all linked or available in the PR

[ ] This PR introduces a breaking change

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@smouillour smouillour self-requested a review February 8, 2018 16:31
@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@romainseb romainseb self-requested a review February 13, 2018 13:21
@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

nicomaligne
nicomaligne suggested changes Feb 15, 2018
View reviewed changes
packages/cmf/src/sagas/collection.js Outdated
export function* waitFor(id, interval = 10) {
// eslint-disable-next-line no-constant-condition
while (true) {
const collection = yield select(state => state.cmf.collections.get(id));
Copy link
Contributor

Choose a reason for hiding this comment

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

Make a dediacted function instead of a anonymous one would be better no ?
By the way i think that you didn't do the TU here.

packages/cmf/src/sagas/component.js
if (!saga) {
invariant(process.env.NODE_ENV !== 'production', `The saga ${action.saga} is not registred`);
} else {
const task = yield fork(saga);
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be a dumb question, but do we really always want to do a fork of the saga ?

output/cmf.eslint.txt
@@ -5,7 +5,7 @@
The react/require-extension rule is deprecated. Please use the import/extensions rule from eslint-plugin-import instead.

/home/travis/build/Talend/ui/packages/cmf/src/cmfConnect.js
151:4 warning Unexpected console statement no-console
152:4 warning Unexpected console statement no-console
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use invariant instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one is for a deprecation warning, invariant throw an error, so it block the app in dev.

@jmfrancois jmfrancois mentioned this pull request Feb 15, 2018
Merged
4 tasks
romainseb
romainseb requested changes Feb 15, 2018
View reviewed changes
packages/cmf/src/api.js Outdated
@@ -38,5 +44,7 @@ export default {
expression,
route,
registry,
registerInternals,
sagas,
Copy link
Contributor

Choose a reason for hiding this comment

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

sagas & saga feel me strange, could be ambiguous
could one of those ( sagas ) be renamed or moved maybe ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

merged into one

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

acateland
acateland suggested changes Feb 15, 2018
View reviewed changes
Copy link
Contributor

@acateland acateland left a comment

Choose a reason for hiding this comment

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

Just to put the PR on hold, giving me time to work out some docs, and maybe change the implementation so testing saga using the putActionCreator will be consitent with other redux-saga api

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

acateland
acateland suggested changes Feb 15, 2018
View reviewed changes
packages/cmf/src/sagas/collection.js
* @param {string} id of the collection to wait for
* @param {number} interval in ms
*/
export function* waitFor(id, interval = 10) {
Copy link
Contributor

Choose a reason for hiding this comment

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

would rename it to waitForCollectionAvailable so it is not mistaken with some low level saga helper like delay(ms, [val])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's already in a 'collection' module :/

acateland
acateland approved these changes Feb 16, 2018
View reviewed changes
Copy link
Contributor

@acateland acateland left a comment

Choose a reason for hiding this comment

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

My updated doc for this will come in another PR

@jmfrancois jmfrancois merged commit 88af045 into master Feb 16, 2018
@jmfrancois jmfrancois deleted the jmfrancois/feat/cmf-add-saga-integration branch February 16, 2018 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romainseb romainseb romainseb approved these changes

@nicomaligne nicomaligne nicomaligne approved these changes

@acateland acateland acateland approved these changes

@smouillour smouillour Awaiting requested review from smouillour

Assignees
No one assigned
Labels
need review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jmfrancois @build-travis-ci @romainseb @nicomaligne @acateland @talend-glorieux