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

Track stats for automatic block conversion #2205

Merged
merged 14 commits into from
Aug 4, 2017

Conversation

nylen
Copy link
Member

@nylen nylen commented Aug 3, 2017

Follow-up to #2135. That PR uses a technique which we should probably try to avoid unless absolutely necessary: transparently converting one block type into another when loading a post.

Let's track how often this happens to get more understanding of how it behaves in real-world usage.

This requires moving stat tracking out of the editor module and into the utils module because otherwise this change would create a circular dependency (blocks → editor → blocks). See previous discussion at #2140 (comment).

'utils',
'components',
Copy link
Member Author

Choose a reason for hiding this comment

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

blocks now depends on utils, so utils should be loaded first. Output from the script at #965:

'blocks' depends on 'components'
'blocks' depends on 'element'
'blocks' depends on 'i18n'
'blocks' depends on 'utils'
'components' depends on 'element'
'components' depends on 'i18n'
'components' depends on 'utils'
'editor' depends on 'blocks'
'editor' depends on 'components'
'editor' depends on 'date'
'editor' depends on 'element'
'editor' depends on 'i18n'
'editor' depends on 'utils'

Suggested load order:

element
i18n
utils
components
blocks
date
editor

Copy link
Contributor

@youknowriad youknowriad Aug 4, 2017

Choose a reason for hiding this comment

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

I think the order is defined by the dependencies definition in the PHP side, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

At one point, this was important to get right in the webpack config because otherwise it would break the test build. It seems this is no longer the case since we moved to Jest. I'll update accordingly.

@nylen nylen force-pushed the update/track-auto-converted-blocks branch 3 times, most recently from 5e41ea8 to a54a6f1 Compare August 4, 2017 00:01
return {
bumpStat,
};
},
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs a better solution (probably a higher-order component), but it was the easiest way I could think of to allow mocking the bumpStat function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use jest.mock? this feels weird.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, so does jest.mock 😄

I'll try it again though.

@nylen nylen added this to the Beta 0.7.0 milestone Aug 4, 2017
@nylen nylen force-pushed the update/track-auto-converted-blocks branch 2 times, most recently from 8b4fc4e to f2bcfde Compare August 4, 2017 15:14
@codecov
Copy link

codecov bot commented Aug 4, 2017

Codecov Report

Merging #2205 into master will increase coverage by 0.4%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2205     +/-   ##
=========================================
+ Coverage   23.83%   24.24%   +0.4%     
=========================================
  Files         142      142             
  Lines        4459     4459             
  Branches      756      756             
=========================================
+ Hits         1063     1081     +18     
+ Misses       2866     2853     -13     
+ Partials      530      525      -5
Impacted Files Coverage Δ
components/button/index.js 90.9% <ø> (ø) ⬆️
editor/enable-tracking-prompt/index.js 100% <ø> (+12.5%) ⬆️
utils/tracking.js 91.3% <ø> (ø)
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
editor/inserter/index.js 0% <0%> (ø) ⬆️
blocks/api/parser.js 100% <100%> (ø) ⬆️
components/popover/index.js 65.21% <0%> (+65.21%) ⬆️

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 87e46bd...be220b6. Read the comment docs.

@nylen nylen force-pushed the update/track-auto-converted-blocks branch from c817e67 to 9f98969 Compare August 4, 2017 17:31
@nylen
Copy link
Member Author

nylen commented Aug 4, 2017

@youknowriad you can see my attempts above at getting Jest mocking to work for testing the call to bumpStat here. It didn't go well - Jest mocks are almost undocumented and I couldn't get it to work. Let's just save that for another day.

Maybe we should switch to refx for analytics: components dispatch a BUMP_STAT action, and this action has an effect of bumping the given stat.

tracking.bumpStat = jest.fn();
document.addEventListener = jest.fn( ( event, cb ) => {
eventMap[ event ] = cb;
} );
Copy link
Member Author

Choose a reason for hiding this comment

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

This approach is from enzymejs/enzyme#426 (comment).

@nylen nylen merged commit d482b4f into master Aug 4, 2017
@nylen nylen deleted the update/track-auto-converted-blocks branch August 4, 2017 21:24
Tug pushed a commit that referenced this pull request May 12, 2020
…e_video_to_5.0.2

Update react-native-video to version 5.0.2
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.

2 participants