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

deps: update to typescript 3.5.3 #9357

Merged
merged 2 commits into from
Jul 12, 2019
Merged

deps: update to typescript 3.5.3 #9357

merged 2 commits into from
Jul 12, 2019

Conversation

brendankenny
Copy link
Member

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.

@@ -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'>}
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 ...

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

tried to improve it :)

@@ -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'>}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@@ -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') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 🤷‍♂

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

patrickhulce commented Jul 12, 2019

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
Copy link
Member Author

brendankenny commented Jul 12, 2019

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
Copy link
Member Author

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
Copy link
Member Author

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants