User context #49

odinuge · 2017-12-27T19:13:02Z

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 with https://github.com/reactjs/reselect, and a state transformed with https://github.com/paularmstrong/normalizr:

import { createSelector } from 'reselect';
export const selectCurrentUser = createSelector(
  state => state.users.byId,
  state => state.auth.id,
  (usersById, userId) => usersById[userId]
);
[...]
createRavenMiddleware(Raven, {
            userContextStateTransformer: selectCurrentUser
})

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 in https://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.setDataCallback is not available.

codecov-io · 2017-12-27T19:15:33Z

Codecov Report

Merging #49 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #49   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          21     24    +3     
  Branches        8      9    +1     
=====================================
+ Hits           21     24    +3

Impacted Files	Coverage Δ
index.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fd666b...d92db68. Read the comment docs.

captbaritone

Looks good! Thanks for suggesting this. Add the docs and i’m happy to merge. FYI I’ll be offline until Saturday.

captbaritone · 2017-12-27T19:51:57Z

index.test.js

+      expect(context.mockTransport.mock.calls[0][0].data.user).toEqual(
+        userData
+      );
+    });


I don’t think this test is really nessesary. The first one like this, in the “default config” test block is probably enough.

Removed the "duplicate" test now

captbaritone · 2017-12-27T19:53:37Z

index.js

@@ -8,15 +8,20 @@ function createRavenMiddleware(Raven, options = {}) {
    actionTransformer = identity,
    stateTransformer = identity,
    breadcrumbCategory = "redux-action",
-    filterBreadcrumbActions = filter
+    filterBreadcrumbActions = filter,
+    userContextStateTransformer


Maybe we could call it getUserContext? That would fit with the convention i’ve seen for naming selectors which, as you show in your example, this is.

Agree! That is a much better name!

odinuge · 2017-12-27T21:33:23Z

That was quick!

Sounds good @captbaritone; have added some initial documentation now.

odinuge · 2017-12-27T21:37:32Z

README.md

+
+Default: `undefined` (disabled)
+
+Signature: `state => userContext`


Not sure if this is the best way to do this, and still be consistent with the others. Since it cannot default to state => undefined; because it should be possible not to use it, and use the Raven.setUserContext elsewhere....

How about we specify the signature as:

#### `getUserContext` _(Optional Function)_

And then omit the “default” line.

Sounds good 👍 Fixed it now 😄

odinuge · 2017-12-27T21:41:07Z

index.js

-      data.extra.state = stateTransformer(store.getState());
+      data.extra.state = stateTransformer(state);
+      if (getUserContext) {
+        data.user = getUserContext(state);


Do you think we should add a note that the original state, and not the transformed one?

You mean, a note about it overwriting any existing user context?

No, but I did indeed add a notice about that.

But, a note saying that the getUserContext is executed with the original redux-state, and not the state transformed via the stateTransformer.

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.

captbaritone · 2017-12-27T22:20:14Z

README.md

+Signature: `state => userContext`
+
+The [user context] is an important part of the error reporting. When
+`getUserContext` is a truthy function, the result of `getUserContext`


All functions are truthy.

Yeah. When setting the function to optional, using truthy doesn't make that much sense.

Maybe just “when getUserContext is specified”?

👍
Did a small rephrase now 😄

captbaritone · 2017-12-27T22:27:00Z

README.md

+
+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.


I think it’s standard to only make the first reference to a given term a link.

captbaritone

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! 👏

captbaritone · 2017-12-30T20:59:38Z

Published in 1.2.0. Thanks for the PR!

captbaritone reviewed Dec 27, 2017

View reviewed changes

Add support for user data callback

9f1285d

odinuge force-pushed the user_context branch from 6ca2626 to cc69624 Compare December 27, 2017 21:32

odinuge commented Dec 27, 2017

View reviewed changes

odinuge mentioned this pull request Dec 27, 2017

[WIP] Fix raven context in node for SSR webkom/lego-webapp#1034

Merged

odinuge force-pushed the user_context branch from cc69624 to 6ee8cbd Compare December 27, 2017 22:08

captbaritone reviewed Dec 27, 2017

View reviewed changes

odinuge added 2 commits December 27, 2017 23:30

Add documentation for user context callback

259e8f2

Add data callback tests

d92db68

odinuge force-pushed the user_context branch from 6ee8cbd to d92db68 Compare December 27, 2017 22:30

captbaritone approved these changes Dec 27, 2017

View reviewed changes

captbaritone merged commit 49f4d15 into captbaritone:master Dec 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User context #49

User context #49

odinuge commented Dec 27, 2017

codecov-io commented Dec 27, 2017 •

edited

Loading

captbaritone left a comment

captbaritone Dec 27, 2017

odinuge Dec 27, 2017

captbaritone Dec 27, 2017

odinuge Dec 27, 2017

odinuge commented Dec 27, 2017

odinuge Dec 27, 2017

captbaritone Dec 27, 2017

odinuge Dec 27, 2017

odinuge Dec 27, 2017

captbaritone Dec 27, 2017

odinuge Dec 27, 2017

captbaritone Dec 27, 2017

captbaritone Dec 27, 2017

odinuge Dec 27, 2017

captbaritone Dec 27, 2017

odinuge Dec 27, 2017

captbaritone Dec 27, 2017

odinuge Dec 27, 2017

captbaritone left a comment

captbaritone commented Dec 30, 2017


		Default: `undefined` (disabled)

		Signature: `state => userContext`

User context #49

User context #49

Conversation

odinuge commented Dec 27, 2017

codecov-io commented Dec 27, 2017 • edited Loading

Codecov Report

captbaritone 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

odinuge commented Dec 27, 2017

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

captbaritone left a comment

Choose a reason for hiding this comment

captbaritone commented Dec 30, 2017

codecov-io commented Dec 27, 2017 •

edited

Loading