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

Addon actions: fix slow logging #3133

Merged
merged 24 commits into from
Apr 16, 2018
Merged

Addon actions: fix slow logging #3133

merged 24 commits into from
Apr 16, 2018

Conversation

rhalff
Copy link
Contributor

@rhalff rhalff commented Mar 2, 2018

All non plain objects will be serialized to a depth of 2.

Events are not treated differently anymore traversing is also cut off at the depth of 2.

All plain objects are still serialized until a depth of 10 (could probably be increased)

Properties starting with double underscore __ and the storybook's STORYBOOK_ vars are not serialized and ignored.

fixes #2590

@codecov
Copy link

codecov bot commented Mar 2, 2018

Codecov Report

Merging #3133 into master will increase coverage by 0.31%.
The diff coverage is 73.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3133      +/-   ##
==========================================
+ Coverage   36.74%   37.05%   +0.31%     
==========================================
  Files         459      463       +4     
  Lines       10144    10219      +75     
  Branches      906      923      +17     
==========================================
+ Hits         3727     3787      +60     
- Misses       5884     5905      +21     
+ Partials      533      527       -6
Impacted Files Coverage Δ
addons/actions/src/index.js 100% <ø> (ø) ⬆️
addons/actions/src/lib/util/prepareArguments.js 72.72% <100%> (+15.58%) ⬆️
addons/actions/src/preview/configureActions.js 100% <100%> (ø)
addons/actions/src/lib/types/object/index.js 79.16% <100%> (-8.34%) ⬇️
addons/actions/src/lib/util/omitProperty.js 75% <100%> (ø)
...ons/actions/src/lib/types/object/configureDepth.js 62.5% <100%> (ø)
addons/actions/src/preview/action.js 76.66% <100%> (ø)
addons/actions/src/lib/util/index.js 100% <100%> (ø) ⬆️
addons/actions/src/preview/decorateAction.js 50% <50%> (ø)
addons/actions/src/lib/decycle.js 59.25% <55.17%> (+5.73%) ⬆️
... and 79 more

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 131250a...bc883bc. Read the comment docs.

@Hypnosphi Hypnosphi added addon: actions performance issue patch:yes Bugfix & documentation PR that need to be picked to main branch labels Mar 3, 2018
@Hypnosphi
Copy link
Member

Hypnosphi commented Mar 4, 2018

Can we make it configurable? I think there might be situations where people may need deeper nesting

@rhalff
Copy link
Contributor Author

rhalff commented Mar 4, 2018

Yes, it possible to make it configurable.

Should I implement this by listening on the channel for options set by the Options addon?

@Hypnosphi
Copy link
Member

Hypnosphi commented Mar 4, 2018

I think exporting a configuration function from actions addon would be more explicit

@stale
Copy link

stale bot commented Mar 30, 2018

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Mar 30, 2018
@stale stale bot removed the inactive label Apr 14, 2018
@rhalff
Copy link
Contributor Author

rhalff commented Apr 14, 2018

Sorry, no code update yet, I was only bringing this branch up to date.

@@ -2,7 +2,6 @@ import reviver from './reviver';
import { muteProperty } from './util';
import { CYCLIC_KEY } from './';

// eslint-disable-next-line no-control-regex
Copy link
Member

Choose a reason for hiding this comment

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

Looks like it's still needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, my ide reported it wasn't, will put it back.

@Hypnosphi Hypnosphi added the bug label Apr 15, 2018
// addons, panels and events get unique names using a prefix
export const ADDON_ID = 'storybook/actions';
export const PANEL_ID = `${ADDON_ID}/actions-panel`;
export const EVENT_ID = `${ADDON_ID}/action-event`;

export * from './preview';
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, unfortunately this export * syntax doesn't play well with transform-runtime babel/babel#2877

@Hypnosphi
Copy link
Member

Can you please add an example with deeply nested object and different depth configuration?

@rhalff
Copy link
Contributor Author

rhalff commented Apr 15, 2018

It seems the performance gain still relies on setting a fixed maxDepth of 2 whenever an object without a name is encountered, I noticed I didn't remove that part yet: https://github.com/rhalff/storybook/blob/b8aee78cb40627b73db339d58306bb5bee48f8ef/addons/actions/src/lib/types/object/index.js#L5-L18

The decision I made there seems a bit arbitrary, but when I remove that part logging something like window will hang again. I'm not sure yet what would be a better way to prevent this.

Perhaps it's also the amount of properties/entries per object and not only the depth which causes the lag.

@Hypnosphi
Copy link
Member

I assume it's ready

@Hypnosphi Hypnosphi merged commit 503c186 into storybookjs:master Apr 16, 2018
@Hypnosphi Hypnosphi added the patch:done Patch/release PRs already cherry-picked to main/release branch label Apr 17, 2018
Hypnosphi added a commit that referenced this pull request Apr 17, 2018
Addon actions: fix slow logging
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: actions bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch performance issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants