-
Notifications
You must be signed in to change notification settings - Fork 9.6k
lightwallet: add path support to budget #8675
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |||||
| * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | ||||||
| */ | ||||||
| 'use strict'; | ||||||
| const URL = require('../lib/url-shim'); | ||||||
|
|
||||||
| class Budget { | ||||||
| /** | ||||||
|
|
@@ -21,14 +22,10 @@ class Budget { | |||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @param {LH.Budget.ResourceBudget} resourceBudget | ||||||
| * @return {LH.Budget.ResourceBudget} | ||||||
| * @return {Array<string>} | ||||||
| */ | ||||||
| static validateResourceBudget(resourceBudget) { | ||||||
| const {resourceType, budget, ...invalidRest} = resourceBudget; | ||||||
| Budget.assertNoExcessProperties(invalidRest, 'Resource Budget'); | ||||||
|
|
||||||
| const validResourceTypes = [ | ||||||
| static validResourceTypes() { | ||||||
| return [ | ||||||
| 'total', | ||||||
| 'document', | ||||||
| 'script', | ||||||
|
|
@@ -39,9 +36,19 @@ class Budget { | |||||
| 'other', | ||||||
| 'third-party', | ||||||
| ]; | ||||||
| if (!validResourceTypes.includes(resourceBudget.resourceType)) { | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @param {LH.Budget.ResourceBudget} resourceBudget | ||||||
| * @return {LH.Budget.ResourceBudget} | ||||||
| */ | ||||||
| static validateResourceBudget(resourceBudget) { | ||||||
| const {resourceType, budget, ...invalidRest} = resourceBudget; | ||||||
| Budget.assertNoExcessProperties(invalidRest, 'Resource Budget'); | ||||||
|
|
||||||
| if (!this.validResourceTypes().includes(resourceBudget.resourceType)) { | ||||||
| throw new Error(`Invalid resource type: ${resourceBudget.resourceType}. \n` + | ||||||
| `Valid resource types are: ${ validResourceTypes.join(', ') }`); | ||||||
| `Valid resource types are: ${this.validResourceTypes().join(', ') }`); | ||||||
| } | ||||||
| if (isNaN(resourceBudget.budget)) { | ||||||
| throw new Error('Invalid budget: ${resourceBudget.budget}'); | ||||||
|
|
@@ -52,6 +59,70 @@ class Budget { | |||||
| }; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @param {string} path | ||||||
| * @return {string} | ||||||
| */ | ||||||
| static validatePath(path) { | ||||||
| if (!path) { | ||||||
| throw new Error(`A valid path must be provided`); | ||||||
| } | ||||||
|
|
||||||
| const hasLeadingSlash = path[0] === '/'; | ||||||
| const validWildcardQuantity = ((path.match(/\*/g) || []).length <= 1); | ||||||
| const validDollarSignQuantity = ((path.match(/\*/g) || []).length <= 1); | ||||||
khempenius marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| const validDollarSignPlacement = (path.indexOf('$') === -1) || (path[path.length - 1] === '$'); | ||||||
|
|
||||||
| const isValid = hasLeadingSlash && validWildcardQuantity | ||||||
| && validDollarSignQuantity && validDollarSignPlacement; | ||||||
|
|
||||||
| if (!isValid) { | ||||||
| throw new Error(`Invalid path ${path}. ` + | ||||||
| `'Path' should be specified using the 'robots.txt' format.\n` + | ||||||
| `Learn more about the 'robots.txt' format here:\n` + | ||||||
| `https://developers.google.com/search/reference/robots_txt#url-matching-based-on-path-values`); | ||||||
| } | ||||||
| return path; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @param {string} url | ||||||
| * @param {string} pattern | ||||||
| * @return {boolean} | ||||||
| */ | ||||||
| static urlMatchesPattern(url, pattern) { | ||||||
khempenius marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| /** | ||||||
| * Pattern should use the robots.txt format. E.g. "/*-article.html" or "/". Reference: | ||||||
| * https://developers.google.com/search/reference/robots_txt#url-matching-based-on-path-values | ||||||
| */ | ||||||
|
|
||||||
| const urlObj = new URL(url); | ||||||
| const urlPath = urlObj.pathname + urlObj.search; | ||||||
|
|
||||||
| const hasWildcard = pattern.includes('*'); | ||||||
khempenius marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| const hasEndingPattern = pattern.includes('$'); | ||||||
|
|
||||||
| // No *, No $: URL should start with given pattern | ||||||
| if (!hasWildcard && !hasEndingPattern) { | ||||||
khempenius marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| return urlPath.startsWith(pattern); | ||||||
| // No *, Yes $: URL should end with given pattern | ||||||
| } else if (!hasWildcard && hasEndingPattern) { | ||||||
| return urlPath.endsWith(pattern.slice(0, -1)); | ||||||
| // Yes *, No $: URL should start with the string pattern that comes before the wildcard | ||||||
| // & later in the string contain the string pattern that comes after the wildcard. | ||||||
| } else if (hasWildcard && !hasEndingPattern) { | ||||||
| const [beforeWildcard, afterWildcard] = pattern.split('*'); | ||||||
| const remainingUrl = urlPath.slice(beforeWildcard.length); | ||||||
| return urlPath.startsWith(beforeWildcard) && remainingUrl.includes(afterWildcard); | ||||||
| // Yes *, Yes $: URL should start with the string pattern that comes before the wildcard | ||||||
| // & end with the string pattern that comes after the wildcard. | ||||||
| } else if (hasWildcard && hasEndingPattern) { | ||||||
| const [beforeWildcard, afterWildcard] = pattern.split('*'); | ||||||
| return urlPath.startsWith(beforeWildcard) && urlPath.endsWith(afterWildcard.slice(0, -1)); | ||||||
| } | ||||||
| return false; | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be impossible right?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be, but TypeScript complains about it missing a return statement without it.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah for sure :) maybe just a comment to that effect or throw even? |
||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @param {LH.Budget.TimingBudget} timingBudget | ||||||
| * @return {LH.Budget.TimingBudget} | ||||||
|
|
@@ -98,31 +169,30 @@ class Budget { | |||||
| /** @type {LH.Budget} */ | ||||||
| const budget = {}; | ||||||
|
|
||||||
| const {resourceSizes, resourceCounts, timings, ...invalidRest} = b; | ||||||
| const {path, resourceSizes, resourceCounts, timings, ...invalidRest} = b; | ||||||
| Budget.assertNoExcessProperties(invalidRest, 'Budget'); | ||||||
|
|
||||||
| if (b.resourceSizes !== undefined) { | ||||||
| budget.resourceSizes = b.resourceSizes.map((r) => { | ||||||
| budget.path = Budget.validatePath(path); | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this doesn't seem to mutate path, WDYT about?
Suggested change
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't. And I may be getting ahead of myself here, but I think budget input will be mutated soon. (Specifically, in #8539 I think converting user budgets from KB to bytes should be handled in budget.js rather than the audit.) Once it does, I personally find it more readable when everything uses the assignment pattern rather than mixing styles. TLDR; I feel like this style is more future-proof?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. gotcha thanks for the explanation that makes sense!
unfortunately I feel like that might be inevitable when we as a reader, I start looking for how this method changes
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, I have an imminent PR that does a little more budget validation that will helpfully make this decision more complicated :/ |
||||||
|
|
||||||
| if (resourceSizes !== undefined) { | ||||||
| budget.resourceSizes = resourceSizes.map((r) => { | ||||||
| return Budget.validateResourceBudget(r); | ||||||
| }); | ||||||
| } | ||||||
|
|
||||||
| if (b.resourceCounts !== undefined) { | ||||||
| budget.resourceCounts = b.resourceCounts.map((r) => { | ||||||
| if (resourceCounts !== undefined) { | ||||||
| budget.resourceCounts = resourceCounts.map((r) => { | ||||||
| return Budget.validateResourceBudget(r); | ||||||
| }); | ||||||
| } | ||||||
|
|
||||||
| if (b.timings !== undefined) { | ||||||
| budget.timings = b.timings.map((t) => { | ||||||
| if (timings !== undefined) { | ||||||
| budget.timings = timings.map((t) => { | ||||||
| return Budget.validateTimingBudget(t); | ||||||
| }); | ||||||
| } | ||||||
| budgets.push({ | ||||||
| resourceSizes, | ||||||
| resourceCounts, | ||||||
| timings, | ||||||
| }); | ||||||
|
|
||||||
| budgets.push(budget); | ||||||
| }); | ||||||
| return budgets; | ||||||
| } | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| [ | ||
| { | ||
| "path": "/", | ||
| "resourceSizes": [ | ||
| { | ||
| "resourceType": "script", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ | |
| "channel": "cli", | ||
| "budgets": [ | ||
| { | ||
| "path": "/", | ||
| "resourceSizes": [ | ||
| { | ||
| "resourceType": "script", | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.