- Notifications
You must be signed in to change notification settings - Fork22
User context#49
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
User context#49
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-io commentedDec 27, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #49 +/- ##===================================== Coverage 100% 100% ===================================== Files 1 1 Lines 21 24 +3 Branches 8 9 +1 =====================================+ Hits 21 24 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good! Thanks for suggesting this. Add the docs and i’m happy to merge. FYI I’ll be offline until Saturday.
index.test.js Outdated
| expect(context.mockTransport.mock.calls[0][0].data.user).toEqual( | ||
| userData | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don’t think this test is really nessesary. The first one like this, in the “default config” test block is probably enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Removed the "duplicate" test now
index.js Outdated
| breadcrumbCategory="redux-action", | ||
| filterBreadcrumbActions=filter | ||
| filterBreadcrumbActions=filter, | ||
| userContextStateTransformer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe we could call itgetUserContext? That would fit with the convention i’ve seen for naming selectors which, as you show in your example, this is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agree! That is a much better name!
That was quick! Sounds good@captbaritone; have added some initial documentation now. |
| Default:`undefined` (disabled) | ||
| Signature:`state => userContext` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure if this is the best way to do this, and still be consistent with the others. Since it cannot default tostate => undefined; because it should be possible not to use it, and use theRaven.setUserContext elsewhere....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How about we specify the signature as:
#### `getUserContext` _(Optional Function)_And then omit the “default” line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sounds good 👍 Fixed it now 😄
| data.extra.state=stateTransformer(store.getState()); | ||
| data.extra.state=stateTransformer(state); | ||
| if(getUserContext){ | ||
| data.user=getUserContext(state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do you think we should add a note that the original state, and not the transformed one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You mean, a note about it overwriting any existing user context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, but I did indeed add a notice about that.
But, a note saying that thegetUserContext is executed with the original redux-state, andnot the state transformed via thestateTransformer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In the docs, as long as we refer to it as “Redux state” I think that’s clear. In the code, I think it’s self explanatory.
README.md Outdated
| Signature:`state => userContext` | ||
| The[user context] is an important part of the error reporting. When | ||
| `getUserContext` is a truthy function, the result of`getUserContext` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
All functions are truthy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah. When setting the function tooptional, using truthy doesn't make that much sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe just “whengetUserContext is specified”?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
Did a small rephrase now 😄
README.md Outdated
| The[user context] is an important part of the error reporting. When | ||
| `getUserContext` is a truthy function, the result of`getUserContext` | ||
| will set the the[user context] before sending the error report. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it’s standard to only make the first reference to a given term a link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good to me! I’ll cut a new release when I get back on the grid this weekend. Thanks for your quick work on this! 👏
Published in 1.2.0. Thanks for the PR! |
Hi,
This adds a way to set the user context with a state transformer. Any thoughts about this? (the name of the new attr is kinda bad, i know). It would be nice to be able to set the user context in the same callback as the state, because most often it only depends on the state. Here is a simple example withhttps://github.com/reactjs/reselect, and a state transformed withhttps://github.com/paularmstrong/normalizr:
I have also added a few more tests in the second commit (now all lines are covered). If this looks good, i can write documentation so we can get it merged.
Thanks for this project! We use it inhttps://github.com/webkom/lego-webapp, and it works like a charm. 😄 We also use it for
raven(the node version), but that version has a more limited API - eg.setDataCallbackis not available.