From b9344a98fe3d01254dfefb2ed3bda08f88f0c544 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20Mar=C3=A9chal?= Date: Tue, 2 Mar 2021 15:49:12 -0500 Subject: [PATCH] core: lazy preference proxy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is possible to register a preference schema to the `PreferenceSchemaProvider` after the initial inversify binding phase. This is problematic when wanting to do that late schema registration and at the same time trying to bind a preference proxy because proxies must be bound early on. This commit fixes the issue by allowing `createPreferenceProxy` to process a promise for its `schema` parameter (now `promisedSchema`) and still return a proxy synchronously. Until the promise resolves, most operations on the proxy will be no-ops. Signed-off-by: Paul Maréchal --- .../preferences/preference-proxy.spec.ts | 315 +++++++++++------- .../browser/preferences/preference-proxy.ts | 134 ++++---- 2 files changed, 262 insertions(+), 187 deletions(-) diff --git a/packages/core/src/browser/preferences/preference-proxy.spec.ts b/packages/core/src/browser/preferences/preference-proxy.spec.ts index c919841a8a225..71ccaff41064e 100644 --- a/packages/core/src/browser/preferences/preference-proxy.spec.ts +++ b/packages/core/src/browser/preferences/preference-proxy.spec.ts @@ -73,167 +73,226 @@ describe('Preference Proxy', () => { getProvider(PreferenceScope.User).markReady(); getProvider(PreferenceScope.Workspace).markReady(); getProvider(PreferenceScope.Folder).markReady(); - console.log('before ready'); try { await prefService.ready; } catch (e) { console.error(e); } - console.log('done'); }); afterEach(() => { }); + // Actually run the test suite with different parameters: + testPreferenceProxy('Synchronous Schema Definition', { asyncSchema: false }); + testPreferenceProxy('Asynchronous Schema Definition (1s delay)', { asyncSchema: true }); + function getProvider(scope: PreferenceScope): MockPreferenceProvider { return testContainer.getNamed(PreferenceProvider, scope) as MockPreferenceProvider; } - function getProxy(schema?: PreferenceSchema, options?: PreferenceProxyOptions): PreferenceProxy<{ [key: string]: any }> { - const s: PreferenceSchema = schema || { - properties: { - 'my.pref': { - type: 'string', - defaultValue: 'foo' + function testPreferenceProxy(testDescription: string, testOptions: { asyncSchema: boolean }): void { + + describe(testDescription, () => { + + function getProxy(schema?: PreferenceSchema, options?: PreferenceProxyOptions): { + proxy: PreferenceProxy<{ [key: string]: any }>, + /** Only set if we are using a schema asynchronously. */ + promisedSchema?: Promise + } { + const s: PreferenceSchema = schema || { + properties: { + 'my.pref': { + type: 'string', + defaultValue: 'foo' + } + } + }; + if (testOptions.asyncSchema) { + const promisedSchema = new Promise(resolve => setTimeout(() => { + prefSchema.setSchema(s); + resolve(s); + }, 1000)); + const proxy = createPreferenceProxy(prefService, promisedSchema, options); + return { proxy, promisedSchema }; + } else { + prefSchema.setSchema(s); + const proxy = createPreferenceProxy(prefService, s, options); + return { proxy }; } } - }; - prefSchema.setSchema(s); - return createPreferenceProxy(prefService, s, options); - } - it('by default, it should get provide access in flat style but not deep', () => { - const proxy = getProxy(); - expect(proxy['my.pref']).to.equal('foo'); - expect(proxy.my).to.equal(undefined); - expect(Object.keys(proxy).join()).to.equal(['my.pref'].join()); - }); + if (testOptions.asyncSchema) { + it('using the proxy before the schema is set should be no-op', async () => { + const { proxy } = getProxy(); + let changed = 0; + proxy.onPreferenceChanged(event => { + changed += 1; + }); + expect(proxy['my.pref']).to.equal(undefined); + expect(proxy.my).to.equal(undefined); + expect(Object.keys(proxy).length).to.equal(0); + // The proxy doesn't know the schema, so events shouldn't be forwarded: + await getProvider(PreferenceScope.User).setPreference('my.pref', 'foo'); + expect(changed).to.equal(0); + expect(proxy['my.pref']).to.equal(undefined); + expect(proxy.my).to.equal(undefined); + expect(Object.keys(proxy).length).to.equal(0); + }); + } - it('it should get provide access in deep style but not flat', () => { - const proxy = getProxy(undefined, { style: 'deep' }); - expect(proxy['my.pref']).to.equal(undefined); - expect(proxy.my.pref).to.equal('foo'); - expect(Object.keys(proxy).join()).to.equal(['my'].join()); - }); + it('by default, it should get provide access in flat style but not deep', async () => { + const { proxy, promisedSchema } = getProxy(); + if (promisedSchema) { + await promisedSchema; + } + expect(proxy['my.pref']).to.equal('foo'); + expect(proxy.my).to.equal(undefined); + expect(Object.keys(proxy).join()).to.equal(['my.pref'].join()); + }); - it('it should get provide access in to both styles', () => { - const proxy = getProxy(undefined, { style: 'both' }); - expect(proxy['my.pref']).to.equal('foo'); - expect(proxy.my.pref).to.equal('foo'); - expect(Object.keys(proxy).join()).to.equal(['my', 'my.pref'].join()); - }); + it('it should get provide access in deep style but not flat', async () => { + const { proxy, promisedSchema } = getProxy(undefined, { style: 'deep' }); + if (promisedSchema) { + await promisedSchema; + } + expect(proxy['my.pref']).to.equal(undefined); + expect(proxy.my.pref).to.equal('foo'); + expect(Object.keys(proxy).join()).to.equal(['my'].join()); + }); - it('it should forward change events', async () => { - const proxy = getProxy(undefined, { style: 'both' }); - let theChange: PreferenceChangeEvent<{ [key: string]: any }>; - proxy.onPreferenceChanged(change => { - expect(theChange).to.equal(undefined); - theChange = change; - }); - let theSecondChange: PreferenceChangeEvent<{ [key: string]: any }>; - (proxy.my as PreferenceProxy<{ [key: string]: any }>).onPreferenceChanged(change => { - expect(theSecondChange).to.equal(undefined); - theSecondChange = change; - }); + it('it should get provide access in to both styles', async () => { + const { proxy, promisedSchema } = getProxy(undefined, { style: 'both' }); + if (promisedSchema) { + await promisedSchema; + } + expect(proxy['my.pref']).to.equal('foo'); + expect(proxy.my.pref).to.equal('foo'); + expect(Object.keys(proxy).join()).to.equal(['my', 'my.pref'].join()); + }); - await getProvider(PreferenceScope.User).setPreference('my.pref', 'bar'); + it('it should forward change events', async () => { + const { proxy, promisedSchema } = getProxy(undefined, { style: 'both' }); + if (promisedSchema) { + await promisedSchema; + } + let theChange: PreferenceChangeEvent<{ [key: string]: any }>; + proxy.onPreferenceChanged(change => { + expect(theChange).to.equal(undefined); + theChange = change; + }); + let theSecondChange: PreferenceChangeEvent<{ [key: string]: any }>; + (proxy.my as PreferenceProxy<{ [key: string]: any }>).onPreferenceChanged(change => { + expect(theSecondChange).to.equal(undefined); + theSecondChange = change; + }); - expect(theChange!.newValue).to.equal('bar'); - expect(theChange!.oldValue).to.equal(undefined); - expect(theChange!.preferenceName).to.equal('my.pref'); - expect(theSecondChange!.newValue).to.equal('bar'); - expect(theSecondChange!.oldValue).to.equal(undefined); - expect(theSecondChange!.preferenceName).to.equal('my.pref'); - }); + await getProvider(PreferenceScope.User).setPreference('my.pref', 'bar'); - it('toJSON with deep', () => { - const proxy = getProxy({ - properties: { - 'foo.baz': { - type: 'number', - default: 4 - }, - 'foo.bar.x': { - type: 'boolean', - default: true - }, - 'foo.bar.y': { - type: 'boolean', - default: false - }, - 'a': { - type: 'string', - default: 'a' - } - } - }, { style: 'deep' }); - assert.deepStrictEqual(JSON.stringify(proxy, undefined, 2), JSON.stringify({ - foo: { - baz: 4, - bar: { - x: true, - y: false - } - }, - a: 'a' - }, undefined, 2), 'there should not be foo.bar.x to avoid sending excessive data to remote clients'); - }); + expect(theChange!.newValue).to.equal('bar'); + expect(theChange!.oldValue).to.equal(undefined); + expect(theChange!.preferenceName).to.equal('my.pref'); + expect(theSecondChange!.newValue).to.equal('bar'); + expect(theSecondChange!.oldValue).to.equal(undefined); + expect(theSecondChange!.preferenceName).to.equal('my.pref'); + }); - it('get nested default', () => { - const proxy = getProxy({ - properties: { - 'foo': { - 'anyOf': [ - { - 'enum': [ - false - ] + it('toJSON with deep', async () => { + const { proxy, promisedSchema } = getProxy({ + properties: { + 'foo.baz': { + type: 'number', + default: 4 }, - { - 'properties': { - 'bar': { - 'anyOf': [ - { - 'enum': [ - false - ] - }, - { - 'properties': { - 'x': { - type: 'boolean' + 'foo.bar.x': { + type: 'boolean', + default: true + }, + 'foo.bar.y': { + type: 'boolean', + default: false + }, + 'a': { + type: 'string', + default: 'a' + } + } + }, { style: 'deep' }); + if (promisedSchema) { + await promisedSchema; + } + assert.deepStrictEqual(JSON.stringify(proxy, undefined, 2), JSON.stringify({ + foo: { + baz: 4, + bar: { + x: true, + y: false + } + }, + a: 'a' + }, undefined, 2), 'there should not be foo.bar.x to avoid sending excessive data to remote clients'); + }); + + it('get nested default', async () => { + const { proxy, promisedSchema } = getProxy({ + properties: { + 'foo': { + 'anyOf': [ + { + 'enum': [ + false + ] + }, + { + 'properties': { + 'bar': { + 'anyOf': [ + { + 'enum': [ + false + ] }, - 'y': { - type: 'boolean' + { + 'properties': { + 'x': { + type: 'boolean' + }, + 'y': { + type: 'boolean' + } + } } - } + ] } - ] + } + } + ], + default: { + bar: { + x: true, + y: false } } } - ], - default: { - bar: { - x: true, - y: false - } } + }, { style: 'both' }); + if (promisedSchema) { + await promisedSchema; } - } - }, { style: 'both' }); - assert.deepStrictEqual(proxy['foo'], { - bar: { - x: true, - y: false - } - }); - assert.deepStrictEqual(proxy['foo.bar'], { - x: true, - y: false + assert.deepStrictEqual(proxy['foo'], { + bar: { + x: true, + y: false + } + }); + assert.deepStrictEqual(proxy['foo.bar'], { + x: true, + y: false + }); + assert.strictEqual(proxy['foo.bar.x'], true); + assert.strictEqual(proxy['foo.bar.y'], false); + }); }); - assert.strictEqual(proxy['foo.bar.x'], true); - assert.strictEqual(proxy['foo.bar.y'], false); - }); + } }); diff --git a/packages/core/src/browser/preferences/preference-proxy.ts b/packages/core/src/browser/preferences/preference-proxy.ts index f3f63b8f9bf8b..0a4dd281a5e86 100644 --- a/packages/core/src/browser/preferences/preference-proxy.ts +++ b/packages/core/src/browser/preferences/preference-proxy.ts @@ -16,7 +16,7 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ -import { Disposable, Event } from '../../common'; +import { Disposable, Event, MaybePromise } from '../../common'; import { PreferenceService } from './preference-service'; import { PreferenceSchema, OverridePreferenceName } from './preference-contribution'; import { PreferenceScope } from './preference-scope'; @@ -151,7 +151,7 @@ export interface PreferenceProxyOptions { * Creates a preference proxy for typesafe preference handling. * * @param preferences the underlying preference service to use for preference handling. - * @param schema the JSON Schema which describes which preferences are available including types and descriptions. + * @param promisedSchema the JSON Schema which describes which preferences are available including types and descriptions. Can be a promise. * @param options configuration options. * * @returns the created preference proxy. @@ -163,32 +163,42 @@ export interface PreferenceProxyOptions { * 3. Bind the return value of `createPreferenceProxy` to make your preferences available wherever needed. * * See {@link CorePreferences} for an example. + * + * Note that if `schema` is a Promise, most actions will be no-ops until the promise is resolved. */ -export function createPreferenceProxy(preferences: PreferenceService, schema: PreferenceSchema, options?: PreferenceProxyOptions): PreferenceProxy { +export function createPreferenceProxy(preferences: PreferenceService, promisedSchema: MaybePromise, options?: PreferenceProxyOptions): PreferenceProxy { const opts = options || {}; const prefix = opts.prefix || ''; const style = opts.style || 'flat'; const isDeep = style === 'deep' || style === 'both'; const isFlat = style === 'both' || style === 'flat'; + let schema: PreferenceSchema | undefined; + if (PreferenceSchema.is(promisedSchema)) { + schema = promisedSchema; + } else { + promisedSchema.then(s => schema = s); + } const onPreferenceChanged = (listener: (e: PreferenceChangeEvent) => any, thisArgs?: any, disposables?: Disposable[]) => preferences.onPreferencesChanged(changes => { - for (const key of Object.keys(changes)) { - const e = changes[key]; - const overridden = preferences.overriddenPreferenceName(e.preferenceName); - const preferenceName: any = overridden ? overridden.preferenceName : e.preferenceName; - if (preferenceName.startsWith(prefix) && (!overridden || !opts.overrideIdentifier || overridden.overrideIdentifier === opts.overrideIdentifier)) { - if (schema.properties[preferenceName]) { - const { newValue, oldValue } = e; - listener({ - newValue, oldValue, preferenceName, - affects: (resourceUri, overrideIdentifier) => { - if (overrideIdentifier !== undefined) { - if (overridden && overridden.overrideIdentifier !== overrideIdentifier) { - return false; + if (schema) { + for (const key of Object.keys(changes)) { + const e = changes[key]; + const overridden = preferences.overriddenPreferenceName(e.preferenceName); + const preferenceName: any = overridden ? overridden.preferenceName : e.preferenceName; + if (preferenceName.startsWith(prefix) && (!overridden || !opts.overrideIdentifier || overridden.overrideIdentifier === opts.overrideIdentifier)) { + if (schema.properties[preferenceName]) { + const { newValue, oldValue } = e; + listener({ + newValue, oldValue, preferenceName, + affects: (resourceUri, overrideIdentifier) => { + if (overrideIdentifier !== undefined) { + if (overridden && overridden.overrideIdentifier !== overrideIdentifier) { + return false; + } } + return e.affects(resourceUri); } - return e.affects(resourceUri); - } - }); + }); + } } } } @@ -207,18 +217,20 @@ export function createPreferenceProxy(preferences: PreferenceService, schema: const ownKeys: () => string[] = () => { const properties = []; - for (const p of Object.keys(schema.properties)) { - if (p.startsWith(prefix)) { - const idx = p.indexOf('.', prefix.length); - if (idx !== -1 && isDeep) { - const pre = p.substr(prefix.length, idx - prefix.length); - if (properties.indexOf(pre) === -1) { - properties.push(pre); + if (schema) { + for (const p of Object.keys(schema.properties)) { + if (p.startsWith(prefix)) { + const idx = p.indexOf('.', prefix.length); + if (idx !== -1 && isDeep) { + const pre = p.substr(prefix.length, idx - prefix.length); + if (properties.indexOf(pre) === -1) { + properties.push(pre); + } + } + const prop = p.substr(prefix.length); + if (isFlat || prop.indexOf('.') === -1) { + properties.push(prop); } - } - const prop = p.substr(prefix.length); - if (isFlat || prop.indexOf('.') === -1) { - properties.push(prop); } } } @@ -232,22 +244,24 @@ export function createPreferenceProxy(preferences: PreferenceService, schema: if (style === 'deep' && property.indexOf('.') !== -1) { return false; } - const fullProperty = prefix ? prefix + property : property; - if (schema.properties[fullProperty]) { - preferences.set(fullProperty, value, PreferenceScope.Default); - return true; - } - const newPrefix = fullProperty + '.'; - for (const p of Object.keys(schema.properties)) { - if (p.startsWith(newPrefix)) { - const subProxy: { [k: string]: any } = createPreferenceProxy(preferences, schema, { - prefix: newPrefix, - resourceUri: opts.resourceUri, - overrideIdentifier: opts.overrideIdentifier, - style - }); - for (const k of Object.keys(value)) { - subProxy[k] = value[k]; + if (schema) { + const fullProperty = prefix ? prefix + property : property; + if (schema.properties[fullProperty]) { + preferences.set(fullProperty, value, PreferenceScope.Default); + return true; + } + const newPrefix = fullProperty + '.'; + for (const p of Object.keys(schema.properties)) { + if (p.startsWith(newPrefix)) { + const subProxy: { [k: string]: any } = createPreferenceProxy(preferences, schema, { + prefix: newPrefix, + resourceUri: opts.resourceUri, + overrideIdentifier: opts.overrideIdentifier, + style + }); + for (const k of Object.keys(value)) { + subProxy[k] = value[k]; + } } } } @@ -259,19 +273,21 @@ export function createPreferenceProxy(preferences: PreferenceService, schema: throw new Error(`unexpected property: ${String(property)}`); } const fullProperty = prefix ? prefix + property : property; - if (isFlat || property.indexOf('.') === -1) { - if (schema.properties[fullProperty]) { - let value; - if (opts.overrideIdentifier) { - value = preferences.get(preferences.overridePreferenceName({ - overrideIdentifier: opts.overrideIdentifier, - preferenceName: fullProperty - }), undefined, opts.resourceUri); - } - if (value === undefined) { - value = preferences.get(fullProperty, undefined, opts.resourceUri); + if (schema) { + if (isFlat || property.indexOf('.') === -1) { + if (schema.properties[fullProperty]) { + let value; + if (opts.overrideIdentifier) { + value = preferences.get(preferences.overridePreferenceName({ + overrideIdentifier: opts.overrideIdentifier, + preferenceName: fullProperty + }), undefined, opts.resourceUri); + } + if (value === undefined) { + value = preferences.get(fullProperty, undefined, opts.resourceUri); + } + return value; } - return value; } } if (property === 'onPreferenceChanged') { @@ -289,7 +305,7 @@ export function createPreferenceProxy(preferences: PreferenceService, schema: if (property === 'toJSON') { return toJSON(); } - if (isDeep) { + if (schema && isDeep) { const newPrefix = fullProperty + '.'; for (const p of Object.keys(schema.properties)) { if (p.startsWith(newPrefix)) {