Add the resource timings pane #5

AdamTReineke · 2018-01-12T05:46:52Z

Adds the resource timings pane which provides a two tables: a summary table and a list of files.

Additionally updates project config files as needed and modifies helpers to be cleaner. See individual commit descriptions for more details, especially dd6c766.

- Added more `npm run ...` commands - No more sourcemaps - Update tslint to allow leading underscores in variable names (for unused variables in the mocks) - Remove the unused webpack test config - Rename the units type in Formatter - Fix a minor typing issue in IPanel - Move and update helpers in resource-timing - Add a tsconfig for the tests project so it can be typechecked properly - Fix tslint and tsc type errors in `button.spec.ts`, `formatter.spec.ts`, `toolbar.spec.ts`, `panel.mock.ts`, `navigation-timing-spec.ts` - Add resource timing tests

AdamTReineke · 2018-01-12T05:53:53Z

Screenshot of the pane injected into GitHub:

#ByDesign

Dadstart · 2018-01-12T16:05:17Z

src/formatter.ts

+    };
+
+type FileSizeUnits = "b" | "Kb" | "Mb";
+


enum #Closed

Dadstart · 2018-01-12T16:14:39Z

src/panels/navigation-timing.ts

@@ -42,6 +42,7 @@ export class NavigationTimingsPanel implements IPanel {
    public getButtons(): Button[] {
        return [new Button({
            parent: this,
+            title: "Duration from navigation to end of load event",
            emoji: "⏱️",


I know we're not localizing these strings, but I'm a huge fan of having any UI strings being defined in a separate file. #Closed

I really want the panels to be as independent as possible. However, I'll move the strings into the default config (around line 15) so they're in one place and could be injected by someone else if they wanted to localize their implementation.

In reply to: 161262870 [](ancestors = 161262870)

Dadstart · 2018-01-12T16:15:29Z

src/panels/resource-timing.ts

+     * The global performance object, can be included in the config to enable injection of a mock object for testing.
+     * This pane only requires the getEntriesByType method
+     */
+    performance: Partial<Performance> & Required<{ getEntriesByType(entryType: string): {} }>;


& [](start = 38, length = 1)

I haven't seen this syntax before #Closed

See Intersection Types here

In reply to: 161263142 [](ancestors = 161263142)

Dadstart · 2018-01-12T16:16:50Z

src/panels/resource-timing.ts

+ * Gets a summary table with default zeroed values.
+ */
+export const getZeroedSummaryTable: () => SummaryRow[] = (): SummaryRow[] => {
+    const zeroValues: SummaryRow = {


I'm seeing the () => = () => pattern all over. What is this doing as opposed to just () =>? #Closed

I'm actually -1 on declaring types on lambdas (which is what this is). It's redundant, and makes it likely to mismatch types eventually (two sources of truth). #Resolved

Dadstart · 2018-01-12T16:17:18Z

src/panels/resource-timing.ts

+    performance: window.performance,
+};
+
+export enum INITIATOR_TYPES {


INITIATOR_TYPES [](start = 12, length = 15)

Is it common practice to have enums be all caps? #Closed

Whoops. The values normally are uppercased, but can't be in this case since the names have other meanings. (Which I'll add a comment to note.)

In reply to: 161263665 [](ancestors = 161263665)

Dadstart · 2018-01-12T16:20:14Z

tsconfig.json

@@ -4,7 +4,7 @@
        "noImplicitAny": true,
        "module": "es2015",
        "target": "es5",
-        "sourceMap": true,
+        "sourceMap": false,
        "listFiles": true,


Removed since it wasn't particularly helpful. Can be added back in later if people need it.

In reply to: 161264406 [](ancestors = 161264406)

Dadstart · 2018-01-12T16:20:35Z

tslint.json

@@ -26,7 +26,8 @@
        "no-implicit-dependencies": [true, "dev"],
        "no-import-side-effect": false,
        "strict-boolean-expressions": [true, "allow-undefined-union"],
-        "interface-over-type-literal": false
+        "interface-over-type-literal": false,
+        "variable-name": [true, "ban-keywords", "check-format", "allow-leading-underscore"]


"allow-leading-underscore" [](start = 64, length = 26)

Why allow leading underscore #Closed

See MockPanel (panel.mock.ts) which needs to implement the constructor and render functions. I don't need the values from required parameters so TSC flags on unused variables. Unused parameters with leading underscores are exempt from this check though. See more at microsoft/TypeScript#9458

In reply to: 161264493 [](ancestors = 161264493)

Dadstart

🕐

JoshuaKGoldberg · 2018-01-12T07:08:16Z

.gitignore

+!injectDemoToolbar.user.js
+!commontypes.d.ts


This is part of your source, so it shouldn't be gitignored. #ByDesign

(if you're doing something fancy about not changing the API, document it) #WontFix

Hence the ! in front. :-)

In reply to: 161152364 [](ancestors = 161152364)

JoshuaKGoldberg · 2018-01-12T07:08:51Z

index.html

 }
 #PTB_buttons li {
    display:inline-block;
    line-height:1.6em;
    margin-left:0.5em;
    padding:0.2em;
    cursor:pointer;
+
+    border:1px solid black;
+    border-bottom: none;


Nit: some styles have a space after the colon; some don't. #WontFix

Won't fix because this is getting moved to JavaScript soon. I'll standardize then.

In reply to: 161152446 [](ancestors = 161152446)

JoshuaKGoldberg · 2018-01-12T07:11:35Z

src/formatter.ts

+            case "Mb":
+                return `${(bytes / (twoExpTen * twoExpTen)).toLocaleString(undefined, LOCALE_STRING_DECIMAL_PLACES)} Mb`;
+            default:
+                throw new Error("unknown unit");


Feels like you should turn off the TSLint rule or find/make a config that allows for exhaustive switches. This code should never be hit, and is just wasted bytes. #ByDesign

By Design, since the value can be passed in from JS code that wouldn't know the type limitations.

In reply to: 161152748 [](ancestors = 161152748)

JoshuaKGoldberg · 2018-01-12T16:49:55Z

src/panels/resource-timing.ts

+            // Add to the All count
+            this.incrementCount(summaryCounts[INITIATOR_TYPES.all], entry);
+
+            const index: number = INITIATOR_TYPES[entry.initiatorType as keyof typeof INITIATOR_TYPES];


Type here should be number | undefined; then you wouldn't have to cast a line down. #Closed

JoshuaKGoldberg · 2018-01-12T16:52:03Z

test/button.spec.ts

+        button.render(container);
+
+        if (container.firstElementChild === null) {
+            throw new Error("Not in DOM");


This error message isn't particularly useful and is repeated in a few places. Suggest you:

Make a common helper function in this PR with a more helpful message

File a task to make it a Chai assertion or use a standard Chai library

Merge this PR #Resolved

…ftoolbar into resource-timing

JoshuaKGoldberg · 2018-01-16T23:30:39Z

src/formatter.ts

@@ -9,10 +12,71 @@ export const DECIMAL_PLACES: number = 2;
 * @param decimalPlaces The number of decimal places to show.
 */
 export const duration: (end: number, start: number, decimalPlaces?: number) => string =
-    (end: number, start: number, decimalPlaces: number = DECIMAL_PLACES): string => {
-        if (isNaN(end) || isNaN(start)) {
+    (end, start, decimalPlaces = DECIMAL_PLACES) => {


As mentioned in person: it's driving me crazy that you have duplicate type declarations here (one explicit, one implicit).

Instead of:

export const duration: (end: number, start: number, decimalPlaces?: number) => string = (end, start, decimalPlaces = DECIMAL_PLACES) => { /* ... */ };

...it would be simpler to write:

export const duration = (end: number, start: number, decimalPlaces?: number): string => { /* ... */ }; ``` #Resolved

Dadstart · 2018-01-17T16:00:52Z

src/formatter.ts

+
+        switch (unit) {
+            case "b":
+                return `${bytes.toLocaleString(undefined, LOCALE_STRING_DECIMAL_PLACES)} b`;


case FileSizeUnits[FileSizeUnits.b]
rather than duplicating the string val #Closed

Dadstart · 2018-01-17T16:02:38Z

src/panels/resource-timing.ts

+
+    const numberOfSummaries: number = 8;
+    const summaryCounts: SummaryRow[] = new Array(numberOfSummaries);
+    summaryCounts[InitiatorTypes.all] = { ...zeroValues, format: "All" };


"All" [](start = 64, length = 6)

InitiatorTypes[InitiatorTypes.all] #Closed

Dadstart · 2018-01-17T16:05:15Z

src/panels/resource-timing.ts

+<tr>
+    <td>${entry.initiatorType}</td>
+    <td title="${entry.name}">${Formatter.pathToFilename(entry.name)}</td>
+    <td class="numeric">${Formatter.sizeToString(entry.transferSize)}</td>


numeric [](start = 15, length = 7)

Are you sure this will be matched as PTB_frame.numeric over a separate .numeric class defined by the library consumer included after the PTB CSS? #Closed

There's no complete solution to this since any styles are at the mercy of the page overriding them. (For example, I don't specify a font face anywhere so I get whatever the page gives us.) Shadow DOM is the ideal protector but it's only in Chrome for now.

In reply to: 162096583 [](ancestors = 162096583)

Dadstart

Adam Reineke added 14 commits January 4, 2018 19:07

First stab at resource timings

3ff6143

pretty numbers

3e02d36

Merge branch 'master' into resource-timing

fd40cdc

Prettier output, helper methods

eba0aa3

Merge branch 'master' into resource-timing

ab78238

Add the pane to the user script

607a334

Merge branch 'master' into resource-timing

0f3476f

Move some new types to a common place, add a formatter for paths

fd4ed71

Formatting fixes and added button titles

4fe607e

Add and update formatter tests

988e988

Bad import

d14c60b

Test the new Button.title property

461ebd1

resource-timing refactor with test stubs

f0f4ff4

AdamTReineke self-assigned this Jan 12, 2018

AdamTReineke requested review from JoshuaKGoldberg and Dadstart January 12, 2018 05:46

Adam Reineke and others added 2 commits January 11, 2018 21:54

Update demo

ff81f86

Merge branch 'master' into resource-timing

71ee9ad

Dadstart reviewed Jan 12, 2018

View reviewed changes

Dadstart suggested changes Jan 12, 2018

View reviewed changes

JoshuaKGoldberg previously approved these changes Jan 12, 2018

View reviewed changes

Address CR feedback

2c2f5e7

Merge branch 'resource-timing' of https://github.com/Microsoft/webper…

68841ea

…ftoolbar into resource-timing

AdamTReineke dismissed JoshuaKGoldberg’s stale review via 68841ea January 16, 2018 23:18

Fix some typing errors

8f42ece

JoshuaKGoldberg reviewed Jan 16, 2018

View reviewed changes

JoshuaKGoldberg previously approved these changes Jan 16, 2018

View reviewed changes

Adam Reineke added 3 commits January 16, 2018 16:17

Remove typedef rule, fix a compile type error in tests

4931c28

Improve npm run check to be comprehensive

57bd900

Dropped a default param when cleaning up typedef

b6fb06c

AdamTReineke dismissed JoshuaKGoldberg’s stale review via b6fb06c January 17, 2018 00:30

Dadstart reviewed Jan 17, 2018

View reviewed changes

Fix keying with enums

3a1bc33

Dadstart approved these changes Jan 17, 2018

View reviewed changes

AdamTReineke merged commit acbb974 into master Jan 17, 2018

AdamTReineke deleted the resource-timing branch January 17, 2018 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the resource timings pane #5

Add the resource timings pane #5

AdamTReineke commented Jan 12, 2018

AdamTReineke commented Jan 12, 2018 •

edited

Loading

Dadstart Jan 12, 2018 •

edited

Loading

Dadstart Jan 12, 2018 •

edited

Loading

AdamTReineke Jan 12, 2018

Dadstart Jan 12, 2018 •

edited

Loading

AdamTReineke Jan 16, 2018

Dadstart Jan 12, 2018 •

edited

Loading

JoshuaKGoldberg Jan 12, 2018 •

edited by AdamTReineke

Loading

Dadstart Jan 12, 2018 •

edited

Loading

AdamTReineke Jan 12, 2018

Dadstart Jan 12, 2018 •

edited

Loading

AdamTReineke Jan 16, 2018

Dadstart Jan 12, 2018 •

edited

Loading

AdamTReineke Jan 16, 2018

Dadstart left a comment

JoshuaKGoldberg Jan 12, 2018 •

edited by AdamTReineke

Loading

JoshuaKGoldberg Jan 12, 2018 •

edited by AdamTReineke

Loading

AdamTReineke Jan 12, 2018

JoshuaKGoldberg Jan 12, 2018 •

edited by AdamTReineke

Loading

AdamTReineke Jan 12, 2018

JoshuaKGoldberg Jan 12, 2018 •

edited by AdamTReineke

Loading

AdamTReineke Jan 12, 2018

JoshuaKGoldberg Jan 12, 2018 •

edited by Dadstart

Loading

JoshuaKGoldberg Jan 12, 2018 •

edited by AdamTReineke

Loading

JoshuaKGoldberg Jan 16, 2018 •

edited by AdamTReineke

Loading

Dadstart Jan 17, 2018 •

edited

Loading

Dadstart Jan 17, 2018 •

edited

Loading

Dadstart Jan 17, 2018 •

edited

Loading

AdamTReineke Jan 17, 2018

Dadstart left a comment

		!injectDemoToolbar.user.js
		!commontypes.d.ts

Add the resource timings pane #5

Add the resource timings pane #5

Conversation

AdamTReineke commented Jan 12, 2018

AdamTReineke commented Jan 12, 2018 • edited Loading

Dadstart Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Dadstart Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dadstart Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dadstart Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

JoshuaKGoldberg Jan 12, 2018 • edited by AdamTReineke Loading

Choose a reason for hiding this comment

Dadstart Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dadstart Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dadstart Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dadstart left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Jan 12, 2018 • edited by AdamTReineke Loading

Choose a reason for hiding this comment

JoshuaKGoldberg Jan 12, 2018 • edited by AdamTReineke Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg Jan 12, 2018 • edited by AdamTReineke Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg Jan 12, 2018 • edited by AdamTReineke Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg Jan 12, 2018 • edited by Dadstart Loading

Choose a reason for hiding this comment

JoshuaKGoldberg Jan 12, 2018 • edited by AdamTReineke Loading

Choose a reason for hiding this comment

JoshuaKGoldberg Jan 16, 2018 • edited by AdamTReineke Loading

Choose a reason for hiding this comment

Dadstart Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Dadstart Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Dadstart Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dadstart left a comment

Choose a reason for hiding this comment

AdamTReineke commented Jan 12, 2018 •

edited

Loading

Dadstart Jan 12, 2018 •

edited

Loading

Dadstart Jan 12, 2018 •

edited

Loading

Dadstart Jan 12, 2018 •

edited

Loading

Dadstart Jan 12, 2018 •

edited

Loading

JoshuaKGoldberg Jan 12, 2018 •

edited by AdamTReineke

Loading

Dadstart Jan 12, 2018 •

edited

Loading

Dadstart Jan 12, 2018 •

edited

Loading

Dadstart Jan 12, 2018 •

edited

Loading

JoshuaKGoldberg Jan 12, 2018 •

edited by AdamTReineke

Loading

JoshuaKGoldberg Jan 12, 2018 •

edited by AdamTReineke

Loading

JoshuaKGoldberg Jan 12, 2018 •

edited by AdamTReineke

Loading

JoshuaKGoldberg Jan 12, 2018 •

edited by AdamTReineke

Loading

JoshuaKGoldberg Jan 12, 2018 •

edited by Dadstart

Loading

JoshuaKGoldberg Jan 12, 2018 •

edited by AdamTReineke

Loading

JoshuaKGoldberg Jan 16, 2018 •

edited by AdamTReineke

Loading

Dadstart Jan 17, 2018 •

edited

Loading

Dadstart Jan 17, 2018 •

edited

Loading

Dadstart Jan 17, 2018 •

edited

Loading