eclipse-theia / theia

Eclipse Theia is a cloud & desktop IDE framework implemented in TypeScript.
http://theia-ide.org
Eclipse Public License 2.0
20.06k stars 2.5k forks source link

The preference extension should be always installed explicitly #667

Open akosyakov opened 7 years ago

akosyakov commented 7 years ago

Since the preference extension was split on the API and extension package, there are no extensions depending on the extension package.

How should we handle such cases?

To reproduce try to build and start Theia for the following package:

{
    "private": true,
    "dependencies": {
        "@theia/navigator": "next"
    },
    "devDependencies": {
        "@theia/cli": "next"
    }
}
akosyakov commented 7 years ago

One option will be to provide stub implementations in API packages that an app does not fail. Such stubs can log warnings that the implementation is not provided.

hexa00 commented 7 years ago

Could this be considered a peerDependency?

Since I tought the reason for the split was to avoid circual deps ?

We could depend on the api and have the extension as a peerDep ?

akosyakov commented 7 years ago

It looks like peerDependencies are not installed automatically. It is used only to warn: https://docs.npmjs.com/files/package.json#peerdependencies.

npm versions 1 and 2 will automatically install peerDependencies if they are not explicitly depended upon higher in the dependency tree. In the next major version of npm (npm@3), this will no longer be the case. You will receive a warning that the peerDependency is not installed instead.

hexa00 commented 7 years ago

Humm could Theia take care of installing them ?

vince-fugnitto commented 5 years ago

@akosyakov I think for something like this we should use the stub implementations (and default values) if the preferences extension is not included.

akosyakov commented 5 years ago

@vince-fugnitto yes, I think it used to work like that, but got broken with all refactorings for last years )

paul-marechal commented 4 years ago

Hm, my take on this is that the current design tried to dance around core actually trying to consume some kind of preference to be configured, but it conflicted with the fact that the implementation for preferences lives in @theia/preferences.

Core already does a lot of things, but required things. Why not having the model for preferences defined in core itself? It would make sense to have some config mechanism builtin inside @theia/core, the @theia/preferences extension would contribute the UI, and @theia/workspace would contribute the workspace and folder scopes.

Extensions wanting to access workspace/folder scoped preferences would use @theia/workspace preference utilities. It would make such an extension depend on workspace, but it makes sense because given our API, if you explicitly ask for a preference in the workspace scope, then it means you need the concept of workspace. Right now these scopes are defined in core, but we should move them out.

Would this make more sense?

akosyakov commented 4 years ago

Why not having the model for preferences defined in core itself?

I thought we already have model there. I'm going to rewrite preference provider to use monaco models to respect dirty editors and fix race conditions with current programmatic update of preference values. It should not go to core. Also currently they depend on filesystem how this dependency could be broken?

akosyakov commented 4 years ago

But I like an idea to avoid requiring installation of preference extension. I think you mentioned some default in memory implementation of preference providers once?

paul-marechal commented 4 years ago

Also currently they depend on filesystem how this dependency could be broken?

I remember you once said that one shouldn't use @theia/filesystem from the backend but directly use node's API. If we are to move at least the fetching of user-scoped preferences into @theia/core, I wonder if we can make some json-rpc service that would do the fs stuff on behalf of the frontend. Looking up something in the user home should be basic enough for core to do?

I think you mentioned some default in memory implementation of preference providers once?

This makes me think that from the browser we could store things in localstorage rather than directly on disk (though localstorage is just the browser storing that for you somehow). It might even make more sense than putting it in the user home.

paul-marechal commented 4 years ago

I'm going to rewrite preference provider to use monaco models to respect dirty editors [...]

I don't fully understand the motivation behind it, but does that require to explicitly depend on monaco packages? Sure would be an issue in core, maybe just bring the interfaces into our source code?

[...] and fix race conditions with current programmatic update of preference values.

Didn't know about this issue, sounds good to me.

akosyakov commented 4 years ago

I don't fully understand the motivation behind it, but does that require to explicitly depend on monaco packages? Sure would be an issue in core, maybe just bring the interfaces into our source code?

Yes, I'm thinking to add TextModelService stub implementation in the core, but real implementation still with be in monaco with monaco editor text models. But I don't see how interfaces will help, now core won't work without monaco extension.

This makes me think that from the browser we could store things in localstorage rather than directly on disk (though localstorage is just the browser storing that for you somehow). It might even make more sense than putting it in the user home.

It can work only if a user always using the same machine, the same browser and the same origin (schema+port+host). If one of them changes user preferences are lost.

paul-marechal commented 4 years ago

Yes, I'm thinking to add TextModelService stub implementation in the core [...]

I'm sorry, but could you explain why preferences need this? In my mind, core would provide basic infrastructure for preferences to work and for extensions to build on top of it (new providers/scopes). Monaco-specific things would be contributed from somewhere else than core.

If one of them changes user preferences are lost.

True, could still be interesting to have a preference scope for storing things at this level, but not a hard requirement. It should still be possible for core to have a backend service managing storage in the user home? This way we don't depend on @theia/filesystem.

akosyakov commented 4 years ago

I'm sorry, but could you explain why preferences need this?

Currently we mutate preferences directly on the disk, if a user has a preference file opened with dirty changes we miss it. It is bogus, we should read from a dirty editor model and update it as well. It’s a first problem. Second that vscode extensions are sending many concurrent preference updates leading to different file stats and conflicts. With monaco models updates will be applied one after another and only then flushed to the disk by monaco.

akosyakov commented 4 years ago

It should still be possible for core to have a backend service managing storage in the user home?

It will have the same issues as current implementation as race conditions. It is solvable but it does not worth it if no one really using it in such configuration, i.e. it would need new code which we have to test, maintain, distribute and so on.