Skip to content
This repository has been archived by the owner on Apr 19, 2024. It is now read-only.

Commit

Permalink
Enable a mechanism for something to inject data into vscode-nls (#42)
Browse files Browse the repository at this point in the history
* initial attempt

* use file

* add ts

* also remove leading slash

* try a different way to import a module

* clean up imports

* use a sync require instead

* don't need this file anymore
  • Loading branch information
TylerLeonhardt committed Aug 8, 2022
1 parent 8d8165b commit 0e2dbdb
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 17 deletions.
67 changes: 62 additions & 5 deletions src/browser/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,25 +5,82 @@

import RAL from '../common/ral';

import { setPseudo, localize, Options, LocalizeInfo } from '../common/common';

import { setPseudo, localize, Options, LocalizeInfo, isString, MessageFormat, isNumber, format, LocalizeFunc } from '../common/common';
export { MessageFormat, BundleFormat, Options, LocalizeInfo, LocalizeFunc, LoadFunc, KeyInfo } from '../common/common';

export function loadMessageBundle(_file?: string) {
let nlsData: { [key: string]: string[] };
try {
// Requiring this file will be intercepted by VS Code and will contain actual NLS data.
// @ts-ignore
nlsData = require('vscode-nls-web-data');

This comment has been minimized.

Copy link
@AviVahl

AviVahl Aug 9, 2022

Contributor

this caused regression when bunding, as this is not a dep of the package.json @TylerLeonhardt

This comment has been minimized.

Copy link
@AviVahl

AviVahl Aug 9, 2022

Contributor

Module not found: Error: Can't resolve 'vscode-nls-web-data' in '//node_modules/vscode-nls/lib/browser'

This comment has been minimized.

Copy link
@TylerLeonhardt

TylerLeonhardt Aug 9, 2022

Author Member

Are you using webpack? Can you do:

'vscode-nls-web-data': 'commonjs vscode-nls-web-data', // ignored because this is injected by the webworker extension host

in the externals just like how you do it for vscode?

This comment has been minimized.

Copy link
@AviVahl

AviVahl Aug 9, 2022

Contributor

Yes, using webpack. I've opened an issue here as well regarding this.
I mapped it in the resolver configuration to false, which gives it an empty object. It's a workaround... I'm not sure needing to adjust all webpack configs of other consuming projects is a justified requirement... It is a regression of past work to make this lib bundling compatible.

This comment has been minimized.

Copy link
@TylerLeonhardt

TylerLeonhardt Aug 9, 2022

Author Member

I agree, but I worry there's no other option. I have no way of telling all bundlers that they need to not rewrite this require statement... yet I need this feature in order for an extension on the web to have localization.

Leaves me in a very tricky spot.

This comment has been minimized.

Copy link
@TylerLeonhardt

TylerLeonhardt Aug 9, 2022

Author Member

Perhaps I remove this in 5.x and then re-add it in a 6.0 with a migration doc item.

This comment has been minimized.

Copy link
@AviVahl

AviVahl Aug 9, 2022

Contributor

not sure why it's required to inject values via the module system. thinking about forward compatibility with native esm is also something that my team has in mind these days. perhaps there's an alternative solution rather than code that requires specific bundling configuration?

This comment has been minimized.

Copy link
@TylerLeonhardt

TylerLeonhardt Aug 9, 2022

Author Member

perhaps there's an alternative solution rather than code that requires specific bundling configuration?

I'm open to suggestions! Injecting values this way was needed due to synchronous assumptions in vscode-nls... In other words, we can only load translations synchronously (on desktop it uses fs.readFileSync). The web equivalent is a require statement (but specifically not an async one)... the other option is a synchronous XMLHttpRequest... but that seems very dubious.

This comment has been minimized.

Copy link
@AviVahl

AviVahl Aug 9, 2022

Contributor

I'd probably use module state for this case. The state will change when someone calls an exported setWhatever fn. Consumers will be able to call this function one time in the entry of their application, injecting data. They can also call it later on to adjust it on the fly. This would also be compatible with both cjs and esm. That's how test runners usually do it.

This comment has been minimized.

Copy link
@TylerLeonhardt

TylerLeonhardt Aug 10, 2022

Author Member

Let me make sure I understand what you're saying...

So we have VS Code's webworker extension host loading and invoking the extension:
https://github.com/microsoft/vscode/blob/a2a836aebffe09cd2c512e80f3819a19b41f02ca/src/vs/workbench/api/worker/extHostExtensionService.ts#L79-L122
The module there is the entrypoint of your extension... but within that extension you have loaded vscode-nls which is what actually needs the data.

So is the idea that the entrypoint calls some other function on vscode-nls that plumbs the data through?

I'm a little skeptical... because some bundlers hoist require statements to the top of the file and my fear is that a loadMessageBundle in a deeper file gets run before this setWhatever function is run... causing random errors to be thrown. This is why we do things like this in the Python extension:
https://github.com/microsoft/vscode-python/blob/main/src/setupNls.ts
and other Webpack extensions... to ensure that config is run before everything else.

This comment has been minimized.

Copy link
@AviVahl

AviVahl Aug 10, 2022

Contributor

Yes, that's the idea. If my memory is correct, vscode-nls already uses this pattern for injection of other things.
Hoisting only occurs to esm imports, not commonjs. Order of evaluation is retained even then. We use this pattern all over js (describe/it in mocha; hooks in react). Main downside (that some dim fair requirement) is that you must have a single copy of the library for this to work.

IMO much less intrusive than using module system and requiring special build configuration.

This comment has been minimized.

Copy link
@AviVahl

AviVahl Aug 10, 2022

Contributor

Btw we get vscode-nls bundled in a use-case outside vscode. We get it through vscode-css-languageservice that we connect to monaco-editor-core in a web application.

Mentioning this because I think it's important to note this lib is used in several contexts...

} catch(e) {
console.error('Loading vscode-nls-web-data failed. Are you running this outside of VS Code? If so, you may need to intercept the import call with your bundled NLS data.');
nlsData = {};
}

interface InternalOptions {
locale: string | undefined;
language: string | undefined;
languagePackSupport: boolean;
cacheLanguageResolution: boolean;
messageFormat: MessageFormat;
languagePackId?: string;
cacheRoot?: string;
}

let options: InternalOptions;

export function loadMessageBundle(file?: string) {
if (!file) {
// No file. We are in dev mode. Return the default
// localize function.
return localize;
}
// Remove extension since we load json files.
if (file.endsWith('.js') || file.endsWith('.ts')) {
file = file.substring(0, file.length - 3);
}
if (file.startsWith('/')) {
file = file.substring(1);
}
if (nlsData && nlsData[file]) {
return createScopedLocalizeFunction(nlsData[file]);
}
return function (key: string | number | LocalizeInfo, message: string, ...args: any[]): string {
if (typeof key === 'number') {
throw new Error(`Browser implementation does currently not support externalized strings.`);
throw new Error('Externalized strings were not present in the environment.');
} else {
return localize(key, message, ...args);
}
};
}

export function config(options?: Options) {
// This API doesn't really do anything in practice because the message bundle _has_ to be loaded
// ahead of time via 'vscode-nls-web-data'.
export function config(opts?: Options) {
setPseudo(options?.locale?.toLowerCase() === 'pseudo');
return loadMessageBundle;
}

function createScopedLocalizeFunction(messages: string[]): LocalizeFunc {
return function (key: any, message: string, ...args: any[]): string {
if (isNumber(key)) {
if (key >= messages.length) {
console.error(`Broken localize call found. Index out of bounds. Stacktrace is\n: ${(<any>new Error('')).stack}`);
return;
}
return format(messages[key], args);
} else {
if (isString(message)) {
console.warn(`Message ${message} didn't get externalized correctly.`);
return format(message, args);
} else {
console.error(`Broken localize call found. Stacktrace is\n: ${(<any>new Error('')).stack}`);
}
}
};
}

RAL.install(Object.freeze<RAL>({
loadMessageBundle: loadMessageBundle,
config: config
Expand Down
2 changes: 1 addition & 1 deletion src/browser/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"extends": "../../tsconfig.base.json",
"compilerOptions": {
"lib": [
"es5",
"es2015",
"dom"
],
"outDir": "../../lib/browser"
Expand Down
10 changes: 10 additions & 0 deletions src/common/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,16 @@ export function isDefined(value: any): boolean {
return typeof value !== 'undefined';
}

const toString = Object.prototype.toString;

export function isNumber(value: any): value is number {
return toString.call(value) === '[object Number]';
}

export function isString(value: any): value is string {
return toString.call(value) === '[object String]';
}

export let isPseudo = false;

export function setPseudo(pseudo: boolean) {
Expand Down
12 changes: 1 addition & 11 deletions src/node/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,21 +10,11 @@ import RAL from '../common/ral';

import {
format, localize, isDefined, setPseudo, isPseudo, MessageFormat, BundleFormat, Options, TranslationConfig, LanguageBundle, LocalizeFunc,
NlsBundle, MetaDataFile, MetadataHeader, I18nBundle, SingleFileJsonFormat, LoadFunc
NlsBundle, MetaDataFile, MetadataHeader, I18nBundle, SingleFileJsonFormat, LoadFunc, isString, isNumber
} from '../common/common';

export { MessageFormat, BundleFormat, Options, LocalizeInfo, LocalizeFunc, LoadFunc, KeyInfo } from '../common/common';

const toString = Object.prototype.toString;

function isNumber(value: any): value is number {
return toString.call(value) === '[object Number]';
}

function isString(value: any): value is string {
return toString.call(value) === '[object String]';
}

function isBoolean(value: any): value is boolean {
return value === true || value === false;
}
Expand Down

0 comments on commit 0e2dbdb

Please sign in to comment.