Skip to content

Stats: Use only defer for script tag#17974

Merged
kraftbj merged 2 commits intomasterfrom
update/stats-script-loading
Dec 7, 2020
Merged

Stats: Use only defer for script tag#17974
kraftbj merged 2 commits intomasterfrom
update/stats-script-loading

Conversation

@kraftbj
Copy link
Copy Markdown
Contributor

@kraftbj kraftbj commented Dec 4, 2020

Fixes https://twitter.com/rick_viscomi/status/1331735748060524551

Changes proposed in this Pull Request:

  • Drop obsolete use of type.
  • Use only defer.
  • Skip PHPCS validation since the script should be enqueued, but that's going to be the beginning of a deep rabbit hole given how old a lot of this code is. :)

Jetpack product discussion

Discussed with perfops in p1607034688079500-slack-C4GAQ900P

Does this pull request change what data or activity we track or use?

n/a

Testing instructions:

  • Logged out on a site connected to Jetpack, ensure no JS errors and that the stats were counted.
  • Should not be via AMP.

Proposed changelog entry for your changes:

  • Stats: Update JavaScript loading to meet modern best practices.

@kraftbj kraftbj added Bug When a feature is broken and / or not performing as intended [Feature] Stats Data Feature that enables users to track their site's traffic and gain insights on popular content. [Focus] Performance [Pri] Normal [Status] Needs Testing We need to add this change to the testing call for this month's release labels Dec 4, 2020
@kraftbj kraftbj self-assigned this Dec 4, 2020
test-case-reminder[bot]
test-case-reminder Bot reviewed Dec 4, 2020
View reviewed changes
Copy link
Copy Markdown

@test-case-reminder test-case-reminder Bot left a comment

Choose a reason for hiding this comment

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

Here are some suggested test cases for this PR.

Stats

  • Visit home page, make sure that page view is recorded in stats
  • Visit home page in AMP view
  • Visit post page in non-AMP and AMP view

If you think that suggestions should be improved please edit the configuration file here. You can also modify/add test-suites to be used in the configuration file.

@kraftbj kraftbj mentioned this pull request Dec 4, 2020
Merged
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Dec 4, 2020 •
edited
Loading

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against b468c8b

@jeherve jeherve added this to the 9.3 milestone Dec 7, 2020
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Testing We need to add this change to the testing call for this month's release labels Dec 7, 2020
@kraftbj kraftbj merged commit 54b3f54 into master Dec 7, 2020
@kraftbj kraftbj deleted the update/stats-script-loading branch December 7, 2020 19:34
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Dec 7, 2020
anomiex added a commit that referenced this pull request Dec 11, 2020
@anomiex
Changelog: add #17974, #17975
c12f157
jeherve pushed a commit that referenced this pull request Jan 5, 2021
@anomiex @jeherve
Changelog: add #17974, #17975
09a5c2d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve approved these changes

+1 more reviewer

@test-case-reminder test-case-reminder[bot] test-case-reminder[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@kraftbj kraftbj

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Stats Data Feature that enables users to track their site's traffic and gain insights on popular content. [Focus] Performance [Pri] Normal

Projects

None yet

Milestone

9.3

Development

Successfully merging this pull request may close these issues.

5 participants

@kraftbj @jetpackbot @jeherve @anomiex @matticbot