Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Extensions api fixes #1233
Extensions api fixes #1233
Changes from all commits
abaacdd
4cb4f5a
1057042
5643e84
9a0378f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be a class? That way there isn't an abstraction boundary being broken by the loader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this being private, I don't think it implements the
InstalledExtension
interface anymore, is that desirable?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you think it should implement? Using
private
field was suggested by @jakolehmThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope..
extension-loader.ts
keeps bothInstalledExtension
list and enabled instances of those.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extension-manager doesn't keep anything.. Its purpose for install & load extensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't
extension-loader
for installing and loading extensions? I would have assumed thatextension-manager
is the manager of truth and then usesloader
to load stuff andstore
to persist data between restarts...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 centralnode_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.