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

Analytics fixes #1319

Merged
merged 4 commits into from Feb 16, 2018
Merged

Analytics fixes #1319

merged 4 commits into from Feb 16, 2018

Conversation

philipwalton
Copy link
Member

R: @jeffposnick @addyosmani @gauntface

Fixes #1309

This PR fixes the error in #1309, which was resulting from workbox-google-analytics setting the replayed request headers as the string '[["Content-Type", "text/plain"]]' instead of the object.

This likely means none of the replays were working, something that ideally should have been caught by an integration test. I'll put together another PR to make sure an integration test exists that ensures the entire process works. (Note, the node tests didn't catch this because we use header mock that didn't error here when they should have. Not sure if anything should be done about that though.)

In the process of manually testing it, I noticed a few other issues that I fixed as well.

  • Failed workbox-background-sync requests that were modified were storing the modified versions, I updated it to store the original.
  • workbox-google-analytics requests that already included a qt param were having that param overridden when the correct behavior should be to update it instead.

/cc: @DavidScales

@@ -124,6 +124,10 @@ class Queue {

let storableRequest;
while (storableRequest = await this._queueStore.getAndRemoveOldestEntry()) {
// Make a copy so the unmodified request can be stored]

Choose a reason for hiding this comment

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

Can you remove the ] from stored]

Copy link

@gauntface gauntface left a comment

Choose a reason for hiding this comment

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

LGTM from me with one nit.

@workbox-pr-bot
Copy link
Collaborator

PR-Bot Size Plugin

Changed File Sizes

File Before After Change GZipped
packages/workbox-background-sync/build/workbox-background-sync.prod.js 3.27 KB 3.52 KB +8% 1.45 KB
packages/workbox-google-analytics/build/workbox-google-analytics.prod.js 2.07 KB 2.12 KB +3% 1.05 KB

New Files

No new files have been added.

All File Sizes

View Table
File Before After Change GZipped
packages/workbox-background-sync/build/workbox-background-sync.prod.js 3.27 KB 3.52 KB +8% 1.45 KB
packages/workbox-broadcast-cache-update/build/workbox-broadcast-cache-update.prod.js 1.07 KB 1.07 KB 0% 573 B
packages/workbox-build/build/_types.js 41 B 41 B 0% 61 B
packages/workbox-build/build/index.js 2.52 KB 2.52 KB 0% 1.06 KB
packages/workbox-cache-expiration/build/workbox-cache-expiration.prod.js 3.41 KB 3.41 KB 0% 1.26 KB
packages/workbox-cacheable-response/build/workbox-cacheable-response.prod.js 594 B 594 B 0% 350 B
packages/workbox-cli/build/app.js 5.09 KB 5.09 KB 0% 1.86 KB
packages/workbox-cli/build/bin.js 2.32 KB 2.32 KB 0% 1.03 KB
packages/workbox-core/build/workbox-core.prod.js 6.48 KB 6.48 KB 0% 2.59 KB
packages/workbox-google-analytics/build/workbox-google-analytics.prod.js 2.07 KB 2.12 KB +3% 1.05 KB
packages/workbox-precaching/build/workbox-precaching.prod.js 5.26 KB 5.26 KB 0% 2.05 KB
packages/workbox-range-requests/build/workbox-range-requests.prod.js 1.64 KB 1.64 KB 0% 807 B
packages/workbox-routing/build/workbox-routing.prod.js 2.77 KB 2.77 KB 0% 1.25 KB
packages/workbox-strategies/build/workbox-strategies.prod.js 3.26 KB 3.26 KB 0% 1.02 KB
packages/workbox-sw/build/workbox-sw.js 1.46 KB 1.46 KB 0% 793 B
packages/workbox-webpack-plugin/build/generate-sw.js 6.41 KB 6.41 KB 0% 2.17 KB
packages/workbox-webpack-plugin/build/index.js 742 B 742 B 0% 470 B
packages/workbox-webpack-plugin/build/inject-manifest.js 8.82 KB 8.82 KB 0% 2.89 KB

Workbox Aggregate Size Plugin

☠️ WARNING ☠️

We are using 155% of our max size budget.

Total Size: 22.7KB
Percentage of Size Used: 155%

Gzipped: 9.1KB

@coveralls
Copy link

coveralls commented Feb 16, 2018

Coverage Status

Coverage increased (+0.01%) to 89.203% when pulling b32169e on analytics-fixes into 8a1f659 on v3.

@philipwalton philipwalton merged commit df8dc74 into v3 Feb 16, 2018
@philipwalton philipwalton deleted the analytics-fixes branch February 16, 2018 02:03
@DavidScales
Copy link

@philipwalton @gauntface working for me now 👍 Thanks guys

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.

None yet

5 participants