Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

feat(loadModule): optional cache bust key #37

Merged
merged 11 commits into from
Mar 17, 2020
Merged

feat(loadModule): optional cache bust key #37

merged 11 commits into from
Mar 17, 2020

Conversation

JAdshead
Copy link
Contributor

@JAdshead JAdshead commented Feb 13, 2020 •
edited
Loading

  • include holocron update

@JAdshead JAdshead requested a review from a team as a code owner February 13, 2020 22:05
@oneamexbot
Copy link
Contributor

oneamexbot commented Feb 13, 2020 •
edited
Loading

Warnings
⚠️

Changes were made to package.json, but not to package-lock.json - Perhaps you need to run npm install?

📊 Bundle Size Report

file name size on disk gzip
app.js 112.7KB 31.4KB
runtime.js 15KB 5.3KB
vendors.js 128.1KB 37.9KB
app~vendors.js 403.5KB 105.2KB
legacy/app.js 119.4KB 33KB
legacy/runtime.js 15KB 5.3KB
legacy/vendors.js 163KB 44.8KB
legacy/app~vendors.js 409.8KB 106.9KB

Generated by 🚫 dangerJS against b27164b

anescobar1991
anescobar1991 previously approved these changes Feb 27, 2020
View reviewed changes
@anescobar1991
Copy link
Member

You should also open a PR to one-app-dev-cdn to stop adding the key there. That way the not-used-in-development key is omitted since it is no longer needed.

@anescobar1991 anescobar1991 mentioned this pull request Feb 27, 2020
Merged
jsmartfo
jsmartfo reviewed Mar 4, 2020
View reviewed changes
src/server/middleware/sendHtml.js
@@ -115,7 +115,7 @@ export function renderModuleScripts({
const { integrity, url: src } = moduleMap.modules[moduleName][bundle];
const { key } = moduleMap;
const additionalAttributes = isDevelopmentEnv ? '' : `integrity="${integrity}"`;
const scriptSource = isDevelopmentEnv ? src : `${src}?key=${key}`;
const scriptSource = isDevelopmentEnv || !key ? src : `${src}?key=${key}`;
Copy link
Member

Choose a reason for hiding this comment

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

are we naming it key?

@PixnBits do your thing ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a discussion between @PixnBits and @10xLaCroixDrinker

@JAdshead
Copy link
Contributor Author

JAdshead commented Mar 4, 2020 •
edited
Loading

Warnings
⚠️
Changes were made to package.json, but not to package-lock.json - Perhaps you need to run npm install?

📊 Bundle Size Report

file name size on disk gzip
app.js 112.7KB 31.4KB
runtime.js 15KB 5.3KB
vendors.js 128.1KB 37.9KB
appvendors.js 403.5KB 105.2KB
legacy/app.js 119.4KB 33KB
legacy/runtime.js 15KB 5.3KB
legacy/vendors.js 163KB 44.8KB
legacy/appvendors.js 409.8KB 106.9KB
Generated by 🚫 dangerJS against 25a547e

package-lock already contains v1.0.0 for holocron

@JAdshead JAdshead merged commit b34b1bd into master Mar 17, 2020
@JAdshead JAdshead deleted the feat/optional-cache-bust-key branch March 17, 2020 17:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jsmartfo jsmartfo jsmartfo left review comments

@10xLaCroixDrinker 10xLaCroixDrinker 10xLaCroixDrinker approved these changes

@nellyk nellyk nellyk approved these changes

@Francois-Esquire Francois-Esquire Francois-Esquire approved these changes

@anescobar1991 anescobar1991 anescobar1991 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@JAdshead @oneamexbot @anescobar1991 @10xLaCroixDrinker @nellyk @jsmartfo @Francois-Esquire