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

Fix setting connector to re-enable saving user settings (cf. #7147) #7601

Merged
merged 2 commits into from Dec 11, 2019
Merged

Fix setting connector to re-enable saving user settings (cf. #7147) #7601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
40bd06b
moved settings plugin to own src file; some related cleanup
telamonian Dec 7, 2019
e4ac8aa
reenables saving settings by adding save method to SettingConnector
telamonian Dec 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
62 changes: 4 additions & 58 deletions packages/apputils-extension/src/index.ts
View file
Open in desktop
Expand Up @@ -9,7 +9,6 @@ import {
JupyterFrontEnd,
JupyterFrontEndPlugin
} from '@jupyterlab/application';

import {
Dialog,
ICommandPalette,
Expand All @@ -18,28 +17,20 @@ import {
WindowResolver,
Printing
} from '@jupyterlab/apputils';

import {
Debouncer,
ISettingRegistry,
IStateDB,
PageConfig,
SettingRegistry,
StateDB,
Throttler,
URLExt
} from '@jupyterlab/coreutils';

import { defaultIconRegistry } from '@jupyterlab/ui-components';

import { PromiseDelegate } from '@lumino/coreutils';

import { DisposableDelegate } from '@lumino/disposable';

import { Palette } from './palette';

import { SettingConnector } from './settingconnector';

import { settingsPlugin } from './settingsplugin';
import { themesPlugin, themesPaletteMenuPlugin } from './themeplugins';

/**
Expand Down Expand Up @@ -86,51 +77,6 @@ const paletteRestorer: JupyterFrontEndPlugin<void> = {
autoStart: true
};

/**
* The default setting registry provider.
*/
const settings: JupyterFrontEndPlugin<ISettingRegistry> = {
id: '@jupyterlab/apputils-extension:settings',
activate: async (app: JupyterFrontEnd): Promise<ISettingRegistry> => {
const { isDisabled } = PageConfig.Extension;
const connector = new SettingConnector(app.serviceManager.settings);

const registry = new SettingRegistry({
connector,
plugins: (await connector.list('active')).values
});

// If there are plugins that have schemas that are not in the setting
// registry after the application has restored, try to load them manually
// because otherwise, its settings will never become available in the
// setting registry.
void app.restored.then(async () => {
const plugins = await connector.list('all');
plugins.ids.forEach(async (id, index) => {
if (isDisabled(id) || id in registry.plugins) {
return;
}

try {
await registry.load(id);
} catch (error) {
console.warn(`Settings failed to load for (${id})`, error);
if (plugins.values[index].schema['jupyter.lab.transform']) {
console.warn(
`This may happen if {autoStart: false} in (${id}) ` +
`or if it is one of the deferredExtensions in page config.`
);
}
}
});
});

return registry;
},
autoStart: true,
provides: ISettingRegistry
};

/**
* The default window name resolver provider.
*/
Expand Down Expand Up @@ -497,13 +443,13 @@ const state: JupyterFrontEndPlugin<IStateDB> = {
const plugins: JupyterFrontEndPlugin<any>[] = [
palette,
paletteRestorer,
print,
resolver,
settings,
settingsPlugin,
state,
splash,
themesPlugin,
themesPaletteMenuPlugin,
print
themesPaletteMenuPlugin
];
export default plugins;

Expand Down
4 changes: 4 additions & 0 deletions packages/apputils-extension/src/settingconnector.ts
View file
Open in desktop
Expand Up @@ -40,5 +40,9 @@ export class SettingConnector extends DataConnector<
};
}

async save(id: string, raw: string): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

🍳 🥚 on my face. Thanks for the fix 👍

await this._connector.save(id, raw);
Copy link
Member

Choose a reason for hiding this comment

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

I have a slight preference toward return this._connector.save(id, raw); but not a strong one. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was going to say "I'm happy to change it just because you asked, @afshin", but then I looked and saw that the function it's wrapping is also async:

jupyterlab/packages/services/src/setting/index.ts

Line 99 in a1fc14a

async save(id: string, raw: string): Promise<void> {

so for consistency's sake, I think save should stick with async/await here.

To be 100% sure, the async and return Promise versions are semantically equivalent: both will immediately return a Promise that resolves upon completion of the wrapped function, right?

Copy link
Member

Choose a reason for hiding this comment

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

Is there an extra promise getting wrapped up in here? Basically I'm wondering if these three are equivalent or if they return a different level of nesting.

save(id: string, raw: string): Promise<void> {
  return this._connector.save(id, raw);
}
async save(id: string, raw: string): Promise<void> {
  return this._connector.save(id, raw);
}
async save(id: string, raw: string): Promise<void> {
  await this._connector.save(id, raw);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I've read over the async/await docs and I've live inspected all three versions in DevTools. Here's my conclusions:

  • From the perspective of a caller, 1, 2, and 3 are identical. All return a singly nested promise that resolves to an undefined.

  • 1 and 2 are actually identical. If a function is already returning a Promise, all adding the async keyword does is allow for the use of await within the function body.

  • 3 might involve one extra step in it's evaluation. Basically, 1/2 just return the inner promise, whereas 3 returns a promise that resolves when the wrapper function finishes. The extra step would then be the implicit return void 0 that the wrapper function calls after it awaits the inner promise.

So at least I can confidently say that no, there's no extra promise getting wrapped up here. I can also say that for our purposes there are no important distinctions between 1/2/3. @afshin Do you still think I should change it?

Copy link
Member

@afshin afshin Dec 10, 2019 •
edited

Choose a reason for hiding this comment

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

Okay, so I think if we are exporting ES2015, 1 is the best answer, based on this experiment. Does that look right to you? You can play with the playground link to confirm.

Screen Shot 2019-12-10 at 21 38 51

https://www.typescriptlang.org/play/?target=2&ssl=22&ssc=1&pln=23&pc=1#code/MYGwhgzhAEBCkFNoG8BQ0PUgTwHbGgDMB7YgCgEoAuaABQCdiBbASwgQB4A3YlgEwB8KdJlFgA7mBYAXOo1bsAdPQQRiILgjIBycaWnaKAbhEYAvqgupQkGAGFmAB2Ls+w0cGK4I0+gFdgaWJ6MgAjRBp4dgp3UUxpAAs2RQB9cPZoAF5odIQTUStREnJqOWY2Th5+ITQ4zBVpP3pcaETktMRFYsp8zELMHHwcsBDShnL2bl5BWLqGppa2iFTcrtIe02h+jEGCcIAvShpxhUrpms2xSRlWpOWOpW7jTe3oR3oWLjBpJAeESMQJjMQA

Copy link
Member

Choose a reason for hiding this comment

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

(This is all very pedantic, but it's good to know what JS we are actually generating.)

}

private _connector: IDataConnector<ISettingRegistry.IPlugin, string>;
}
61 changes: 61 additions & 0 deletions packages/apputils-extension/src/settingsplugin.ts
View file
Open in desktop
@@ -0,0 +1,61 @@
/*-----------------------------------------------------------------------------
| Copyright (c) Jupyter Development Team.
| Distributed under the terms of the Modified BSD License.
|----------------------------------------------------------------------------*/

import {
JupyterFrontEnd,
JupyterFrontEndPlugin
} from '@jupyterlab/application';
import {
ISettingRegistry,
PageConfig,
SettingRegistry
} from '@jupyterlab/coreutils';

import { SettingConnector } from './settingconnector';

/**
* The default setting registry provider.
*/
export const settingsPlugin: JupyterFrontEndPlugin<ISettingRegistry> = {
id: '@jupyterlab/apputils-extension:settings',
activate: async (app: JupyterFrontEnd): Promise<ISettingRegistry> => {
const { isDisabled } = PageConfig.Extension;
const connector = new SettingConnector(app.serviceManager.settings);

const registry = new SettingRegistry({
connector,
plugins: (await connector.list('active')).values
});

// If there are plugins that have schemas that are not in the setting
// registry after the application has restored, try to load them manually
// because otherwise, its settings will never become available in the
// setting registry.
void app.restored.then(async () => {
const plugins = await connector.list('all');
plugins.ids.forEach(async (id, index) => {
if (isDisabled(id) || id in registry.plugins) {
return;
}

try {
await registry.load(id);
} catch (error) {
console.warn(`Settings failed to load for (${id})`, error);
if (plugins.values[index].schema['jupyter.lab.transform']) {
console.warn(
`This may happen if {autoStart: false} in (${id}) ` +
`or if it is one of the deferredExtensions in page config.`
);
}
}
});
});

return registry;
},
autoStart: true,
provides: ISettingRegistry
};