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

Redux saga monitor as a seperate package #1

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

Conversation

lutangar
Copy link
Owner

Following the discussion redux-saga/redux-saga#5 a sample logger (or monitor) was created by @yelouafi as an example for redux-saga.

In the latter discussion, moving the monitor to a seperate module with some more tests was discussed.
As today, such package doesn't exists yet, and I wanted to get rid off the sagaMonitor from my project's sources.

This monitor is nearly identical to the one available in https://github.com/yelouafi/redux-saga sources, see : https://github.com/yelouafi/redux-saga/blob/master/examples/sagaMonitor/index.js

Notable differences

  1. A helper function was added to manipulate the verbosity flag:
createSagaMonitor({ verbose: true })
  1. Instead of the homemadeisPrimitive function, this module https://github.com/jonschlinkert/is-primitive was used. (First match on npm, that's it really... 😅)
  2. Basic tests were added (Any suggestions on the path to take to test this further would be appreciated)
  3. Building tools were added following redux-saga structure.

Disclaimer

I really don't want to offend anyone while packaging this. I didn't put much work on this compared to the people engaged in the discussion (@Splact @davej), and tried to reference authors wherever possible (@sompylasar @romseguy).
I would gladly yield the ownership of this to @yelouafi instead of maintaining this myself 😄

I'll wait for some review from you guys, and some kind of approval from @yelouafi before merging/publishing anything.

@yelouafi
Copy link

Sorry for the late reply. First thanks for taking the example into a separate package.

I plan to start soon working on an UI version of the monitor. Right now I don't have time to review this, but if it works you can publish it if you want. Althouh I'd prefer to be in name different than redux-saga-monitor (so when the UI version is released we could use it)


// Export the snapshot-logging function to run from the browser console or extensions.
if(IS_BROWSER) {
window.$$LogSagas = logSaga

Choose a reason for hiding this comment

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

Please choose a more unique global name, such as __REDUX_SAGA_MONITOR_LOG_SAGAS__. Please see the reasoning here: zalmoxisus/redux-devtools-extension#202

If you're in doubt that the long name would be difficult to run from the browser developer tools console, the console has got a decent auto-complete and command history, so you will only need to enter (read: copy-paste) that once.

}

function createSagaMonitor({ verbose = false } = {}) {
VERBOSE = verbose;

Choose a reason for hiding this comment

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

This function assumes it is only called once. Please wrap the whole context into it.

Please see https://gist.github.com/sompylasar/5e7157e451f4b7268def9ae1ce01edd4 – I would appreciate much if you used that code instead, and just mentioned me as a contributor. Thanks in advance.

}

// Export the snapshot-logging function for arbitrary use by external code.
export { logSaga, createSagaMonitor };

Choose a reason for hiding this comment

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

The logSaga function is context-dependent, it captures the outer context, so it should be in the "saga monitor" that createSagaMonitor creates.

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.

3 participants