Extensions api fixes #1233
Extensions api fixes #1233
Conversation
Signed-off-by: Roman <ixrock@gmail.com>
ixrock
added
bug
Signed-off-by: Roman <ixrock@gmail.com>
Signed-off-by: Roman <ixrock@gmail.com>
| readonly manifest: LensExtensionManifest; | ||
| readonly manifestPath: string; | ||
| readonly isBundled: boolean; | ||
|
|
||
| @observable private isEnabled = false; |
There was a problem hiding this comment.
With this being private, I don't think it implements the InstalledExtension interface anymore, is that desirable?
There was a problem hiding this comment.
Why you think it should implement? Using private field was suggested by @jakolehm
There was a problem hiding this comment.
Maybe I don't have a grasp of how these types interact, I would have assumed that the extension-manager.ts file keeps a record to instances of this type. Does it not?
There was a problem hiding this comment.
Nope.. extension-loader.ts keeps both InstalledExtension list and enabled instances of those.
There was a problem hiding this comment.
IMO that seems very strange, because we are keeping two lists of information that should always be the same. Why not just one list?
There was a problem hiding this comment.
Extension-manager doesn't keep anything.. Its purpose for install & load extensions.
There was a problem hiding this comment.
Isn't extension-loader for installing and loading extensions? I would have assumed that extension-manager is the manager of truth and then uses loader to load stuff and store to persist data between restarts...
There was a problem hiding this comment.
About private: without private extension instance itself cold toggle this which does not make any sense to me and is not really desirable. Why would we allow extension to do this?
ExtensionLoader: it's only purpose is to load (read: execute) extension instances. List of extensions are given to this class, it does not care how that list has generated.
ExtensionManager: it's only purpose is to gather a list of extensions and then install those to central node_modules folder (including all dependencies). Note: this does not load (or create) extension instances.
Naming things is one of the most difficult things in computer science so we probably could have better names for these. Still I strongly believe that separating complex things to smaller units is better than having one giant class that tries to do everything.
| readonly manifest: LensExtensionManifest; | ||
| readonly manifestPath: string; | ||
| readonly isBundled?: boolean; // defined in package.json | ||
| enabled?: boolean; | ||
| } | ||
|
|
There was a problem hiding this comment.
I want to comment on Line 25 and maybe this should be done in a follow up PR (make that a certainly as to not hold up this PR too much) but I am very confused about ExtensionManager since it doesn't actually hold on to a list of extensions that are loaded.
Signed-off-by: Roman <ixrock@gmail.com>
Signed-off-by: Roman <ixrock@gmail.com>
|
Yes, maybe, but afaik it was desire of @jakolehm to keep these parts separated (manager, loader & store). |
Changes:
extensions-store.ts