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

Enable translations for extension code for the web #155355

Merged
merged 2 commits into from
Aug 8, 2022

Conversation

TylerLeonhardt
Copy link
Member

This PR fixes #

Depends on microsoft/vscode-nls#42

In short, the strategy is to load the message bundles ahead of time and then when the extension is getting loaded, return the pre-loaded strings.

The strategies are slightly different because the scanners are slightly different:

  • Built-in only has a map that contains the current locale and the default English.
  • Marketplace extensions has a map of all NLS bundle files in the package

Please review this very harshly. This is code I am unfamiliar with and is owned by several folks.

sandy081
sandy081 previously approved these changes Jul 26, 2022
Copy link
Member

@sandy081 sandy081 left a comment

Choose a reason for hiding this comment

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

LGTM

alexdima
alexdima previously approved these changes Jul 27, 2022
Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

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

Looks super reasonable! I just left a couple of comments. Once you tackle those, could we please create a build with this so we could test it?

src/vs/workbench/api/worker/extHostExtensionService.ts Outdated Show resolved Hide resolved
src/vs/workbench/api/worker/extHostExtensionService.ts Outdated Show resolved Hide resolved
@TylerLeonhardt TylerLeonhardt dismissed stale reviews from alexdima and sandy081 via b0e4241 July 28, 2022 18:07
Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

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

👍

@TylerLeonhardt TylerLeonhardt marked this pull request as ready for review August 8, 2022 16:10
@TylerLeonhardt
Copy link
Member Author

Actually this PR doesn't take advantage of vscode-nls 5.1.0 yet so this can just be merged in independently

@TylerLeonhardt TylerLeonhardt merged commit 49394cc into main Aug 8, 2022
@TylerLeonhardt TylerLeonhardt deleted the tyler/translations-for-web-in-code branch August 8, 2022 16:11
@VSCodeTriageBot VSCodeTriageBot added this to the August 2022 milestone Aug 8, 2022
joyceerhl pushed a commit that referenced this pull request Aug 10, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Sep 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants