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

[telamonian]

References

followup #7147

In the latest master, you cannot change/save any of the user settings. This PR fixes that.

Code changes

Digging through the merged PRs, it looks like in #7147 a save method was left out of the SettingConnector that now wraps the preexisting SettingManager. I added SettingConnector.save, which just forwards SettingManager.save.

I also did a little bit of related code cleanup in apputils-extension, and moved the settings plugin out into its own src file (mostly for the sake of making it easier for me to grasp what had broken)

User-facing changes

Unbreaks settings

Backwards-incompatible changes

None

[jupyterlab-dev-mode]

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link:

[telamonian]

@afshin I was a little lost at first going through the changes in #7174, but your earlier discussion with @vidartf made things a lot clearer:

I thought about putting it in services, but it seemed very specific to the functionality of the SettingRegistry and it seemed to me that the services package should be completely agnostic about what you're doing with the data being returned from the back-end.

One other question I had, though: why does SettingConnector wrap SettingManager instead of, say, subclassing it (since both are themselves subclasses of DataConnector<ISettingRegistry.IPlugin, string>)? Is there any particular reason to prefer one over the other?

[afshin]

why does SettingConnector wrap SettingManager instead of, say, subclassing it (since both are themselves subclasses of DataConnector<ISettingRegistry.IPlugin, string>)? Is there any particular reason to prefer one over the other?

The reasoning was that I didn't want to need to know the SettingManager instantiation options and worry if they'd ever change in the future and instead wanted to rely on the already extant version in the services package.

[afshin]

🍳 🥚 on my face. Thanks for the fix 👍

[afshin]

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

[telamonian]

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?

[afshin]

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);
}

[telamonian]

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?

[afshin]

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.

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

[afshin]

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

[afshin]

👍

[telamonian]

I don't want it to delay getting a fix in for the settings bug, but it would be neat to get someone familiar with the deep magics of the JS runtime to weigh in as to whether there's any difference whatsoever in practice between the different ways to wrap an async function like save. In Python, you can always just inspect the bytecode, but I don't know what the equivalent is for JS.