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 4.7 #14058

Merged
merged 6 commits into from Jun 8, 2022
Merged

deps: update to typescript 4.7 #14058

merged 6 commits into from Jun 8, 2022

Conversation

brendankenny
Copy link
Member

typescript 4.6 and typescript 4.7 have been released since we last updated, so it seemed time to update.

Unlike most updates, we only had a single new error that we needed to fix, and there's nothing new that our code could really benefit from, so it's a pretty simple update. There are some nice updates in typescript 4.6 that people may want to take advantage of (like still being able to discriminate between objects in a union even after the objects are destructured), and typescript 4.7 should make us more resilient in the future if more dependencies start doing complicated package.json exports.

We can wait to land this for typescript-eslint to officially support 4.7 so we don't get the version warning in yarn lint, but that should be today or tomorrow.

@brendankenny brendankenny requested a review from a team as a code owner May 26, 2022 17:53
@brendankenny brendankenny requested review from adamraine and removed request for a team May 26, 2022 17:53
@@ -57,8 +57,8 @@ export class SwapLocaleFeature {
const optionLocaleDisplay = new Intl.DisplayNames([locale], {type: 'language'});

const optionLocaleName = optionLocaleDisplay.of(locale);
const currentLocaleName = currentLocaleDisplay.of(locale);
if (optionLocaleName !== currentLocaleName) {
const currentLocaleName = currentLocaleDisplay.of(locale) || locale;
Copy link
Member Author

Choose a reason for hiding this comment

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

this was the only new "error". new Intl.DisplayNames().of() can return undefined, and optionEl.textContent is supposed to only take string|null, so that was the incompatibility.

In reality, I believe new Intl.DisplayNames only returns null if fallback: 'none' is set, otherwise it always falls back to locale, so this fallback is mostly for tsc's sake, and just in case I'm reading the spec wrong.

Comment on lines 11 to 12
"outDir": "./.tmp/tsbuildinfo/",
"rootDir": ".",
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 was looking at the outDirs in all the tsconfigs recently and was kind of annoyed, but it turns out if I had just looked at the rootDir docs I would have realized it only affects emit, not includes, excludes, etc, so specifying this once in the base tsconfig works perfectly. Makes the extending tsconfigs much nicer, and should fix your problem in c55da8b, @connorjclark

@@ -1,8 +1,6 @@
{
"extends": "../tsconfig-base.json",
"compilerOptions": {
"outDir": "../../.tmp/tsbuildinfo/shared/",
Copy link
Member Author

Choose a reason for hiding this comment

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

checking the diff on the outDir also revealed a minor bug here: this one was saving one directory too high. Check the parent directory of your lighthouse repo and there will be a .tmp in there, too. Sorry :)

"emitDeclarationOnly": true,
"declarationMap": true,
"outDir": "./.tmp/tsbuildinfo/",
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: i'm recently of the opinion that leading ./ in paths are pointless noise. if a path is not starting with / I know it is relative :)

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 don't really care, it just bothers me that rootDir: '' isn't the identity instead of '.' in this case, since they're right next to each other :)

@brendankenny
Copy link
Member Author

I also took a second look at this workaround for the flow report:

rollupPlugins.typescript({
tsconfig: 'flow-report/tsconfig.json',
// Plugin struggles with custom outDir, so revert it from tsconfig value
// as well as any options that require an outDir is set.
outDir: null,
composite: false,
emitDeclarationOnly: false,
declarationMap: false,
}),

with the latest version of @rollup/plugin-typescript, you can get away with removing the outDir and emitDeclarationOnly overrides there, but only if you've already run yarn type-check so everything is already cached in .tmp/tsbuildinfo, so it's probably not worth it. If ui-strings.js and localized-strings.js were typescript files it might work, but it seems to not handle having these js files in the output when it's a composite compile.

To really fix it we probably just need it to get closer to a real build tsconfig, and having those emitDeclarationOnly for js files in there is kind of confusing. Alternatively we could just build it ourselves to a tmp directory and run rollup on the output there.

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.

None yet

4 participants