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(typescript): update to 3.8.3 #10461

Merged
merged 9 commits into from
Apr 8, 2020
Merged

deps(typescript): update to 3.8.3 #10461

merged 9 commits into from
Apr 8, 2020

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Mar 13, 2020

3.5.3 -> 3.7.5 (3.6.* had no errors, 3.8.* had a few more)

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#37-breaking-changes
EDIT: ooooh their new design is dark by default https://www.typescriptlang.org/v2/docs/handbook/release-notes/typescript-3-7.html

  1. change an Object to any.
  2. global type behavior for renderer code changed:
if (typeof module !== 'undefined' && module.exports) {
  // Just for testing in node...
  module.exports = DetailsRenderer;
} else {
  // Used to add in global types
  self.DetailsRenderer = DetailsRenderer;
}

image

Works again if just self.DetailsRenderer = DetailsRenderer;. Tried a few different variants of what we have today, but it seems that any setting of module.exports (despite if guards) causes the module to not be globally resolvable. So, let's just be more explicit and import them as needed (which we do already in other files ....):

/** @typedef {import('./details-renderer.js')} DetailsRenderer */

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#37-breaking-changes

  1. change an Object to any.

It me :) microsoft/TypeScript#32829

  1. global type behavior for renderer code changed:

So, let's just be more explicit and import them as needed (which we do already in other files ....):
/** @typedef {import('./details-renderer.js')} DetailsRenderer */

Ah, this is way better. I believe this means you can also delete this whole chunk of declarations:

interface Window {
CategoryRenderer: typeof _CategoryRenderer;
CriticalRequestChainRenderer: typeof _CriticalRequestChainRenderer;
SnippetRenderer: typeof _SnippetRenderer;
DetailsRenderer: typeof _DetailsRenderer;
DOM: typeof _DOM;
I18n: typeof _I18n;
PerformanceCategoryRenderer: typeof _PerformanceCategoryRenderer;
PwaCategoryRenderer: typeof _PwaCategoryRenderer;
ReportRenderer: typeof _ReportRenderer;
ReportUIFeatures: typeof _ReportUIFeatures;
Util: typeof _Util;
prepareLabData: typeof _prepareLabData;
}

(the global ones have to stick around, though)

@@ -42,7 +42,7 @@ class LanternEstimatedInputLatency extends LanternMetric {

/**
* @param {LH.Gatherer.Simulation.Result} simulation
* @param {Object} extras
* @param {any} extras
Copy link
Member

Choose a reason for hiding this comment

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

we're not dealing with these safely anyways, and unfortunately the templated-class solution doesn't work in tsc because these are static methods, so what do you think of one of

  • using @param {{optimistic: boolean}} extras here (and in the other lantern metrics) and @ts-ignore on the two accesses below or
  • typing extras exactly as what's used and @ts-ignore the call to getEstimateFromSimulation in lantern-metric.js

I think the intent is clearer either way, and it gives a clearer TODO of what needs to be resolved (on our side or the tsc side) to get extras properly checked.

lighthouse-core/lib/cdt/SDK.js Outdated Show resolved Hide resolved
lighthouse-core/test/gather/gather-runner-test.js Outdated Show resolved Hide resolved
@brendankenny
Copy link
Member

3.5.3 -> 3.7.5 (3.6.* had no errors, 3.8.* had a few more)

Why not go all the way to 3.8.3? Seems better to do it all if we're going to make an upgrade

@connorjclark
Copy link
Collaborator Author

Why not go all the way to 3.8.3? Seems better to do it all if we're going to make an upgrade

Just trying to keep the PR smaller. We can do it all at once if you like.

@brendankenny
Copy link
Member

Why not go all the way to 3.8.3? Seems better to do it all if we're going to make an upgrade

Just trying to keep the PR smaller. We can do it all at once if you like.

probably makes sense? It seems like most of the 3.8 errors can also be fixed in a few lines. Could be separate commits for easier reviewing 🤷‍♀

@connorjclark
Copy link
Collaborator Author

It me :) microsoft/TypeScript#32829

wow big props for making a TS change.

weird, says that was for 3.8 but i saw breakage in 3.7


Updated to 3.8.3

@connorjclark connorjclark changed the title deps(typescript): update to 3.7.5 deps(typescript): update to 3.8.3 Mar 19, 2020
Co-Authored-By: Brendan Kenny <bckenny@gmail.com>
@@ -512,6 +512,7 @@ describe('.gotoURL', () => {
}
const replayConnection = new ReplayConnection();

// @ts-ignore: Coerce to TestDriver for looser typing in function parameters.
Copy link
Member

@brendankenny brendankenny Mar 19, 2020

Choose a reason for hiding this comment

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

since this is a real driver, it seems a little weird to cast to a fake driver type :)

One option: could ts-ignore an assertion on loadOptions instead

const loadOptions = {
  waitForLoad: true,
  /** @type {LH.Gatherer.PassContext} */
  // @ts-ignore - we don't need the entire context for the test.
  passContext: {
    passConfig: {
      pauseAfterLoadMs: 0,
      networkQuietThresholdMs: 0,
      cpuQuietThresholdMs: 0,
    },
  },
};

(though we should probably eventually have a passContext generator if we're really getting all real in this file)

@@ -133,7 +133,7 @@ async function flushAllTimersAndMicrotasks(ms = 1000) {
* @property {(...args: RecursivePartial<Parameters<Driver['goOnline']>>) => ReturnType<Driver['goOnline']>} goOnline
*/

/** @typedef {Driver & DriverMockMethods} TestDriver */
/** @typedef {Omit<Driver, keyof DriverMockMethods> & DriverMockMethods} TestDriver */
Copy link
Member

Choose a reason for hiding this comment

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

👍 nice

lighthouse-core/report/html/renderer/snippet-renderer.js Outdated Show resolved Hide resolved
* @property {LH.Artifacts.LanternMetric} fcpResult
* @property {LH.Artifacts.LanternMetric} fmpResult
* @property {LH.Artifacts.LanternMetric} interactiveResult
* @property {{speedIndex: number}} speedline
Copy link
Member

Choose a reason for hiding this comment

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

should these all (except optimistic) be optional, though?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@brendankenny
Copy link
Member

@connorjclark any news? tsc 3.9 release is creeping up fast :)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM! ⌨️☑️

@@ -85,7 +94,7 @@ class LanternMetricArtifact {
/**
* @param {LH.Artifacts.MetricComputationDataInput} data
* @param {LH.Audit.Context} context
* @param {any=} extras
* @param {Omit<Extras, 'optimistic'>=} extras
Copy link
Member

Choose a reason for hiding this comment

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

since requirements aren't changing very quickly any more, at some point we should take a pass at these extras to see if we can untangle them a bit :)

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

Successfully merging this pull request may close these issues.

4 participants