deps: update to typescript 3.5.3 #9357

brendankenny · 2019-07-12T01:43:28Z

unblocks #9267, maybe others?

3.4 crashed on our code base due to out of control unionings. 3.5 introduced a limit to union construction, but the underlying error just morphed into "Expression produces a union type that is too complex to represent" :)

The main cause was microsoft/TypeScript#30769, improving soundness of assigning to an object with enumerated keys (e.g. our artifacts object, where the error was occurring).

The problem is that tsc doesn't have a notion of a type being a single member of a union (e.g. when looping over the artifact keys, inside the loop it sees the union of all artifact keys, not a single key within that union). So while that change improved soundness, our unsound (due to tsc's limitations, but, notably, still correct) assignments to artifacts[keyof artifacts] caused a new error that had been ignored before. Whatever value was written to that property had to match the constraints of all possible types of artifacts[keyof artifacts] simultaneously (it had to be a ScriptElements and a Manifest and a StartUrl and a ViewportDimensions and a...), which caused tsc to give up while trying to find the intersection of all those constraints (which the RHS wouldn't have matched anyways).

See that PR for a long discussion on the benefits and the ways the downsides could be worked around. I'm excited about the oneof proposal.

@ts-ignore seemed like the best approach for these, since it leaves us open for removing sometime in the future if there becomes a way to express what we mean.

Compilation time over 3.2 decreases slightly, by about 10% on my machine.

brendankenny · 2019-07-12T01:45:20Z

lighthouse-core/computed/network-analysis.js

@@ -12,7 +12,7 @@ const NetworkRecords = require('./network-records.js');
 class NetworkAnalysis {
  /**
   * @param {Array<LH.Artifacts.NetworkRequest>} records
-   * @return {Omit<LH.Artifacts.NetworkAnalysis, 'throughput'>}
+   * @return {StrictOmit<LH.Artifacts.NetworkAnalysis, 'throughput'>}


caused by microsoft/TypeScript#30552. See that thread for the drama over strictness :)

wow, that is some good drama. I personally found microsoft/TypeScript#30825 more enjoyable to follow :)

Kinda nuts it's a useless version of Omit for me, I got excited for a second I wouldn't have to copy this everywhere

microsoft/TypeScript#30825 (comment) lol

wow, that is some good drama. I personally found microsoft/TypeScript#30825 more enjoyable to follow :)

oh yeah, whoops, that's a way better link

connorjclark · 2019-07-12T03:00:05Z

lighthouse-core/report/html/renderer/report-ui-features.js

-    if (this._copyAttempt) {
+    // Only handle copy button presses (e.g. ignore the user copying page text) and
+    // when the clipboard is available.
+    if (this._copyAttempt && e.clipboardData) {


I've seen this error for a long time in VS Code. Did only newer TS emit this error? maybe my editor is set to typescript@next ...

I've seen this error for a long time in VS Code. Did only newer TS emit this error? maybe my editor is set to typescript@next ...

they change the browser definitions from time to time, so it's possible. If you do the "Select typescript version" command in vs code do you have "Use VS Code's version" selected?

patrickhulce

LGTM

patrickhulce · 2019-07-12T02:37:58Z

lighthouse-core/gather/gather-runner.js

@@ -425,11 +425,11 @@ class GatherRunner {
    for (const [gathererName, phaseResultsPromises] of resultsEntries) {
      try {
        const phaseResults = await Promise.all(phaseResultsPromises);
-        // Take last defined pass result as artifact.
+        // Take last defined pass result as artifact. If there is none, handled by the undefined check below.


feels like not quite a sentence :)

maybe something like?

Suggested change

// Take last defined pass result as artifact. If there is none, handled by the undefined check below.

// Take last defined pass result as artifact. The undefined case is handled by the undefined check below.

tried to improve it :)

patrickhulce · 2019-07-12T02:52:36Z

lighthouse-core/computed/network-analysis.js

@@ -12,7 +12,7 @@ const NetworkRecords = require('./network-records.js');
 class NetworkAnalysis {
  /**
   * @param {Array<LH.Artifacts.NetworkRequest>} records
-   * @return {Omit<LH.Artifacts.NetworkAnalysis, 'throughput'>}
+   * @return {StrictOmit<LH.Artifacts.NetworkAnalysis, 'throughput'>}


wow, that is some good drama. I personally found microsoft/TypeScript#30825 more enjoyable to follow :)

Kinda nuts it's a useless version of Omit for me, I got excited for a second I wouldn't have to copy this everywhere

lighthouse-core/report/html/renderer/report-ui-features.js

patrickhulce · 2019-07-12T02:59:11Z

lighthouse-core/config/config.js

@@ -182,11 +182,9 @@ function cleanFlagsForSettings(flags = {}) {
  const settings = {};

  for (const key of Object.keys(flags)) {
-    // @ts-ignore - intentionally testing some keys not on defaultSettings to discard them.
-    if (typeof constants.defaultSettings[key] !== 'undefined') {


I seem to remember being very careful about in vs undefined for constants.defaultSettings but that might have just been on the delete vs. = undefined side.

the PR (#4960) doesn't seem to suggest there was anything special about this, and in should be even more inclusive for our purposes, so we're probably fine 🤷‍♂

I seem to remember being very careful about in vs undefined for constants.defaultSettings but that might have just been on the delete vs. = undefined side.

yeah, I was thinking since it's just the constants this should be ok (we won't have set anything to undefined to try to mark them as unset), but I can also switch it to an undefined check. It doesn't help the compiler either way since we have to do @ts-ignore on the next line regardless :)

patrickhulce · 2019-07-12T03:02:46Z

Did something happen to the test set of files? travis failed on test-lantern 😬

EDIT: works fine for me locally, might be travis specific

brendankenny · 2019-07-12T20:24:33Z

Did something happen to the test set of files? travis failed on test-lantern 😬

EDIT: works fine for me locally, might be travis specific

hmm, looks like the test files aren't downloading. Didn't this happen once before? I don't remember what we did (if anything).

Lantern files affected!
Downloading test set...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  3278    0  3278    0     0    107      0 --:--:--  0:00:30 --:--:--   766
gzip: stdin: not in gzip format
tar: Child returned status 1
tar: Error is not recoverable: exiting now
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
The command "yarn test-lantern" exited with 1.

brendankenny · 2019-07-12T20:27:45Z

Did something happen to the test set of files? travis failed on test-lantern 😬

hmm, looks like the test files aren't downloading.

oh, that's what you were saying :) I thought you meant did this PR change any of the files that trigger the tests

brendankenny · 2019-07-12T20:43:52Z

Downloading test set...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   377    0   377    0     0     12      0 --:--:--  0:00:30 --:--:--    97
  0     0    0 41.4M    0     0  1391k      0 --:--:--  0:00:30 --:--:-- 1391k

just needed some time off, I guess :)

deps: update to typescript 3.5.3

a67ec7f

brendankenny requested review from patrickhulce and paulirish as code owners July 12, 2019 01:43

googlebot added the cla: yes label Jul 12, 2019

brendankenny commented Jul 12, 2019

View reviewed changes

connorjclark reviewed Jul 12, 2019

View reviewed changes

patrickhulce approved these changes Jul 12, 2019

View reviewed changes

feedback

36ff428

vercel bot deployed to staging July 12, 2019 20:28 View deployment

brendankenny merged commit 43b96d4 into master Jul 12, 2019

brendankenny deleted the tsc3.5 branch July 12, 2019 22:10

brendankenny mentioned this pull request Jul 12, 2019

deps: update to open from opn #9267

Merged

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: update to typescript 3.5.3 #9357

deps: update to typescript 3.5.3 #9357

	// Take last defined pass result as artifact. If there is none, handled by the undefined check below.
	// Take last defined pass result as artifact. The undefined case is handled by the undefined check below.

deps: update to typescript 3.5.3 #9357

deps: update to typescript 3.5.3 #9357

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment