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

Pass wrapped function name to mobx action #18

Merged
merged 2 commits into from Aug 13, 2019

Conversation

@cyclops26
Copy link
Contributor

commented Aug 11, 2019

This allows for simpler debugging when using mobx-logger. Without this patch mobx-task actions appear in the log as: "ACTION task.". This allows the logged action to appear similar to a natively logged mobx action.

Pass wrapped function name to mobx action
This allows for simpler debugging when using mobx-logger. Without this patch mobx-task actions appear in the log as: "ACTION task.<unnamed action>". This allows the logged action to appear similar to a natively logged mobx action.
@jeffijoe
Copy link
Owner

left a comment

Hi Eli! Thanks for the PR! I left a comment about a single change I'd like made before merging. Thanks!

src/task.js Outdated
@@ -118,7 +119,7 @@ function setupTask(fn, taskState, taskStateSchemaKeys, opts) {
* Assigns the given properties to the task.
* E.g. task.setState({ state: 'resolved', result: 1337 })
*/
setState: action(opts => {
setState: action(opts.wrappedFnName, opts => {

This comment has been minimized.

Copy link
@jeffijoe

jeffijoe Aug 11, 2019

Owner

Rather do it here than tack it on to opts, as that would require a typings update too.

Suggested change
setState: action(opts.wrappedFnName, opts => {
setState: action(fn.name || 'unnamed function', opts => {

This comment has been minimized.

Copy link
@cyclops26

cyclops26 Aug 12, 2019

Author Contributor

@jeffijoe that was my first thought as well; however, at least in my testing, fn has already been replaced with the task wrapper at the point and thus fn.name points to the task wrapper function instead of the original function that is being wrapped in the task.

I'm open to a cleaner solution as this wasn't my first preference because it is a little messy looking, but it seemed a slightly more streamlined way to get the data their without multiple new arguments being passed through the functions. And adding a const seemed inappropriate for something this minor.

Is there a way to reference the wrapped function from the wrapper function without delving into using prototypes?

This comment has been minimized.

Copy link
@jeffijoe

jeffijoe Aug 12, 2019

Owner

Actually, why are we even setting it on the task's setState? 😮

This comment has been minimized.

Copy link
@cyclops26

cyclops26 Aug 12, 2019

Author Contributor

To what are you referring? The first parameter of an action needs to either be the named function or a string name of the function passed as the second parameter in order to compatible with mobx-logger.

This comment has been minimized.

Copy link
@jeffijoe

jeffijoe Aug 12, 2019

Owner

Yes but setState is not the fn, which is where you are setting it

This comment has been minimized.

Copy link
@jeffijoe

jeffijoe Aug 13, 2019

Owner

Because a Task has internal state that is observable, and to update observables you need to do it in an action.

This comment has been minimized.

Copy link
@cyclops26

cyclops26 Aug 13, 2019

Author Contributor

Ahh ok - because they are being mixed. Got it. I will adjust this PR to just be the "setState" added to the action(. I will work separately on figuring out a better solution for general task capture into log.

This comment has been minimized.

Copy link
@cyclops26

cyclops26 Aug 13, 2019

Author Contributor

Changes made to PR.

This comment has been minimized.

Copy link
@jeffijoe

jeffijoe Aug 13, 2019

Owner

Released as 1.0.4, thanks!

This comment has been minimized.

Copy link
@cyclops26

cyclops26 Aug 14, 2019

Author Contributor

Thanks! I was hoping to implement a custom I*RunInfo with spyReportStart and spyReportEnd the same way that action implements its event record creation. Sadly, this was apparently removed from the API between MOBX 3 and 4. So guess for now this is the best that can be without getting messy.

@jeffijoe jeffijoe merged commit a36323e into jeffijoe:master Aug 13, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.