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

Switching to a db name instead of just a store name #1184

Merged
merged 4 commits into from Jan 18, 2018

Conversation

gauntface
Copy link

R: @jeffposnick @philipwalton

Previously I was using a single database name with multiple store names for the cache expiration timestamps,. This doesn't work / make sense given that you need to know the store names up front which for multiple instances of the class - will not be the case.

This change moves to using a name for the database and store name to ensure uniqueness.

Fixes #1175

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 176ced6 on cache-expiration-db-name into ** on v3**.

Copy link
Contributor

@jeffposnick jeffposnick left a comment

Choose a reason for hiding this comment

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

That general change seems fine, but are there any considerations necessary to account for developers migrating from either v2 or an older v3 alpha?

If this will just cause those developers to "start fresh" with an empty set of cache expiration data, I think that's fine, but I just wanted to make sure that there isn't the potential to end up with runtime exceptions in that scenario.

@gauntface
Copy link
Author

@jeffposnick I've bumped the version number and added tests to handle a version change. (Also got coverage to 100%).

@workbox-pr-bot
Copy link
Collaborator

PR-Bot Size Plugin

Changed File Sizes

File Before After Change GZipped
packages/workbox-cache-expiration/build/workbox-cache-expiration.prod.js 3.29 KB 3.41 KB +4% 1.27 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.21 KB 3.21 KB 0% 1.38 KB
packages/workbox-broadcast-cache-update/build/workbox-broadcast-cache-update.prod.js 1.06 KB 1.06 KB 0% 567 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.29 KB 3.41 KB +4% 1.27 KB
packages/workbox-cacheable-response/build/workbox-cacheable-response.prod.js 598 B 598 B 0% 350 B
packages/workbox-cli/build/app.js 4.57 KB 4.57 KB 0% 1.65 KB
packages/workbox-cli/build/bin.js 2.55 KB 2.55 KB 0% 1.08 KB
packages/workbox-core/build/workbox-core.prod.js 6.40 KB 6.40 KB 0% 2.54 KB
packages/workbox-google-analytics/build/workbox-google-analytics.prod.js 1.92 KB 1.92 KB 0% 1.00 KB
packages/workbox-precaching/build/workbox-precaching.prod.js 5.18 KB 5.18 KB 0% 2.02 KB
packages/workbox-range-requests/build/workbox-range-requests.prod.js 1.66 KB 1.66 KB 0% 816 B
packages/workbox-routing/build/workbox-routing.prod.js 2.77 KB 2.77 KB 0% 1.26 KB
packages/workbox-strategies/build/workbox-strategies.prod.js 3.24 KB 3.24 KB 0% 1.01 KB
packages/workbox-sw/build/workbox-sw.js 1.46 KB 1.46 KB 0% 796 B
packages/workbox-webpack-plugin/build/generate-sw.js 5.84 KB 5.84 KB 0% 2.04 KB
packages/workbox-webpack-plugin/build/index.js 742 B 742 B 0% 470 B
packages/workbox-webpack-plugin/build/inject-manifest.js 7.95 KB 7.95 KB 0% 2.65 KB

Workbox Aggregate Size Plugin

☠️ WARNING ☠️

We are using 154% of our max size budget.

Total Size: 22.53KB
Percentage of Size Used: 154%

Gzipped: 9.02KB

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 2e32fd6 on cache-expiration-db-name into ** on v3**.

@gauntface gauntface merged commit 84c16b5 into v3 Jan 18, 2018
@gauntface gauntface deleted the cache-expiration-db-name branch January 18, 2018 20:39
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

4 participants