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.

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

The preference extension should be always installed explicitly #667

Open
akosyakov opened this issue Oct 19, 2017 · 15 comments
Open

The preference extension should be always installed explicitly #667

akosyakov opened this issue Oct 19, 2017 · 15 comments
Labels
extension system issues related to the extension system preferences issues related to preferences question user / developer questions

Comments

@akosyakov
Copy link
Member

akosyakov commented Oct 19, 2017

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 akosyakov added the question user / developer questions label Oct 19, 2017
@akosyakov
Copy link
Member Author

akosyakov commented Oct 19, 2017

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
Copy link

hexa00 commented Oct 19, 2017

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 akosyakov added the extension system issues related to the extension system label Oct 30, 2017
@akosyakov
Copy link
Member Author

akosyakov commented Nov 3, 2017

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
Copy link

hexa00 commented Nov 3, 2017

Humm could Theia take care of installing them ?

@vince-fugnitto
Copy link
Member

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

@akosyakov
Copy link
Member Author

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

@vince-fugnitto vince-fugnitto added the preferences issues related to preferences label Nov 18, 2019
@paul-marechal
Copy link
Member

paul-marechal commented Feb 3, 2020

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
Copy link
Member Author

akosyakov commented Feb 4, 2020

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
Copy link
Member Author

akosyakov commented Feb 4, 2020

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
Copy link
Member

paul-marechal commented Feb 4, 2020

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
Copy link
Member

paul-marechal commented Feb 4, 2020

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
Copy link
Member Author

akosyakov commented Feb 4, 2020

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
Copy link
Member

paul-marechal commented Feb 4, 2020

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
Copy link
Member Author

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
Copy link
Member Author

akosyakov commented Feb 5, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension system issues related to the extension system preferences issues related to preferences question user / developer questions
Projects
None yet
Development

No branches or pull requests

4 participants