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.

Sign up for GitHub

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

Jump to bottom

Upgrade the Theia build to use Typescript 5.4.5 #13628

Merged
merged 5 commits into from May 22, 2024
Merged

Upgrade the Theia build to use Typescript 5.4.5 #13628

merged 5 commits into from May 22, 2024

Conversation

tsmaeder
Copy link
Contributor

@tsmaeder tsmaeder commented Apr 19, 2024 •
edited

What it does

Upgrades the Theia build to use Typescript 5.4.5

Fixes #13627

Most updates are just typing to accommodate stricter checks in 5.4.5.

Contributed on behalf of STMicroelectronics

How to test

Run Theia and observe the logs for any uncommon events. The one "real" change is in the nls extractor, I tested that it still generates a reasonable file using this incantation:

theia nls-extract -o ./packages/core/i18n/nls.json -e vscode -f ./packages/**/browser/**/*.{ts,tsx}

@msujew can you chime in if that is sufficient testing for that script or if not, how to test?

Follow-ups

Review checklist

Reminder for reviewers

@tsmaeder tsmaeder changed the title Upgrades the Theia build to use Typescript 5.4.5 Upgrade the Theia build to use Typescript 5.4.5 Apr 19, 2024
@JonasHelming
Copy link
Contributor

Does this fix #13627?

msujew
msujew reviewed Apr 23, 2024
View reviewed changes
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

For some reason, this breaks the nls-extract call for me. I have managed to fix this by exposing the full path to the TypeScript API here:

-const fileLocalization = await extractFromFile(file, content, errors, options);
+const fileLocalization = await extractFromFile(filePath, content, errors, options);

@tsmaeder
Copy link
Contributor Author

Probably best to merge this after the release.

@msujew
Copy link
Member

msujew commented Apr 29, 2024

@tsmaeder Do you mind applying my recommendation? Afterwards, this PR should be good to go.

rschnekenbu
rschnekenbu reviewed May 6, 2024
View reviewed changes
Copy link
Contributor

@rschnekenbu rschnekenbu left a comment

Choose a reason for hiding this comment

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

From what I could test (building and running electron & browser example, that PR is good for me. I did not test the part already covered by Mark.

@tsmaeder
Copy link
Contributor Author

@msujew finally getting back to this. For me, doing npx theia nls-extract -o foo.txt at the base of the theia repo works just fine. What are you doing differently?

@tsmaeder
Upgrades the Theia build to use Typescript 5.4.5
59c4bfd 
Most updagrades are just typing to accomodate stricter checks in 5.4.5.

Contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
@tsmaeder
Fix linter problems
427e2da 
Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
@tsmaeder
Updated yarn.lock after rebasing.
eb8e4ec 
Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
@tsmaeder tsmaeder force-pushed the 13627_upgrade_typescript_5.4.5 branch from cf4428f to eb8e4ec Compare May 20, 2024 13:23
@msujew
Copy link
Member

msujew commented May 21, 2024

@tsmaeder On Ubuntu, the yarn theia nls-extract -o ./packages/core/i18n/nls.json -e vscode -f ./packages/**/browser/**/*.{ts,tsx} command in Theia's root yields me {}. With my change in place, it works as expected.

@tsmaeder
Copy link
Contributor Author

@msujew unfortunately, your change seems to break the extraction on Windows.

@tsmaeder
Copy link
Contributor Author

It works for me, though if I replace the command line with

npx theia nls-extract -o ./foo.txt -e vscode -f packages/**/browser/**/*.{ts,tsx}

Note the missing leading ./ on the glob pattern.

@msujew
Copy link
Member

msujew commented May 22, 2024 •
edited

@tsmaeder In that case the automated translation needs to be adjusted. That's alright.

@tsmaeder
Fix localization-extractor.
bed512a 
Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
@tsmaeder
Copy link
Contributor Author

@msujew I think I've found a fix.

@msujew
Copy link
Member

msujew commented May 22, 2024

It's now working correctly on Ubuntu, but using a leading ./ breaks on Windows and it again yields {}. Wouldn't it just make sense to strip the leading ./, since that seems to work on both Ubuntu and Windows?

@tsmaeder
Copy link
Contributor Author

@msujew theia nls-extract -o foo.txt -e vscode -f ./packages/**/browser/**/*.{ts,tsx} works fine for me on Windows. What's your command line?

@msujew
Copy link
Member

msujew commented May 22, 2024 •
edited

@tsmaeder I'm using yarn theia nls-extract -o foo.txt -e vscode -f "./packages/**/browser/**/*.{ts,tsx}":

image

@tsmaeder
Copy link
Contributor Author

This exact command line (copy/paste) works for me. I even switched to PowerShell.

msujew
msujew approved these changes May 22, 2024
View reviewed changes
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Interesting. It works on Ubuntu so I'm alright with it. We can change the nls-localize stuff once someone complains :D

@tsmaeder
Update Changelog.md
43aef58 
Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
@tsmaeder tsmaeder merged commit 7286049 into eclipse-theia:master May 22, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rschnekenbu rschnekenbu rschnekenbu approved these changes

@msujew msujew msujew approved these changes

Assignees
No one assigned
Labels
None yet
Projects
PR Backlog
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade to Typescript 5.4
4 participants
@tsmaeder @JonasHelming @msujew @rschnekenbu