-
Notifications
You must be signed in to change notification settings - Fork 887
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
[infra] Upgrade and standardize TypeScript to ~4.7.4 #3116
Conversation
🦋 Changeset detectedLatest commit: 509382d The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultslit-element-list
render
update
update-reflect
lit-html-kitchen-sink
render
update
nop-update
lit-html-repeat
render
update
lit-html-template-heavy
render
update
reactive-element-list
render
update
update-reflect
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple non-blocking comments.
@@ -2001,17 +2001,17 @@ const tests = (test: uvu.Test<uvu.Context>, options: ts.CompilerOptions) => { | |||
|
|||
const baseOptions = () => { | |||
const options = ts.getDefaultCompilerOptions(); | |||
options.target = ts.ScriptTarget.ESNext; | |||
options.module = ts.ModuleKind.ESNext; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it okay that we're resorting to default module kind here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In both places where we call this (right below), we now set the module kind, so I think it's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't yet investigated - but when running
npm run test
locally, got a non blocking message❌ [packages/labs/cli:test:actual] Failed with exit status 1
.
Hmm, I'm not able to reproduce this right now. I'm doing the following, does this repro for you the same way? Anything else in the log?
cd lit
git clean -dffx
npm ci
cd packages/labs/cli
npm test
@@ -14,7 +14,7 @@ | |||
}, | |||
"devDependencies": { | |||
"@vitejs/plugin-vue": "^2.3.1", | |||
"typescript": "^4.6.4", | |||
"typescript": "^4.7.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Should this be ~
instead of ^
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
I haven't yet investigated - but when running npm run test
locally, got a non blocking message ❌ [packages/labs/cli:test:actual] Failed with exit status 1
.
I've tried this a number of times recently on 2 machines, and still can't repro. CI hasn't been affected either. It's possible there is some race condition, but I'm just going to merge and keep an eye out. |
~4.7.4
.~
is used instead of^
because TypeScript doesn't follow semver.typescript
out of thedevDependencies
for most packages, so that they just rely on the one in the top-levelpackage.json
. This is to make it more likely we'll upgrade all packages at once in the future, instead of one at a time.@lit/ts-transformers
,@lit/localize-tools
, and@lit-labs/analyzer
includetypescript
in their production dependencies, so those get a changeset and will get releases.@lit/ts-transformers
needed a fix in the tests so that the "legacy" output emitsES2021
instead ofESNext
, since it will include static class initializer blocks otherwise.@lit-labs/analyzer
test had to be upgraded to due some minor change in the way a generated type union was formatted.node_modules/
folders.