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

Stats: Use only defer for script tag #17974

Merged
merged 2 commits into from
Dec 7, 2020
Merged

Conversation

kraftbj
Copy link
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 [Type] 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
Copy link

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

@jetpackbot
Copy link

jetpackbot commented Dec 4, 2020

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
jeherve pushed a commit that referenced this pull request Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Stats Data Feature that enables users to track their site's traffic and gain insights on popular content. [Focus] Performance [Pri] Normal [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants