Fix extension loader race conditions #1815
Conversation
Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
jakolehm
added
bug
| const bundledExtensions = await this.loadBundledExtensions(); | ||
|
|
||
| await this.installPackages(); // install in-tree as a separate step | ||
| const localExtensions = await this.loadFromFolder(this.localFolderPath); | ||
| await this.installPackages(this.packageJsonPath, bundledExtensions); |
There was a problem hiding this comment.
We install only bundled extensions here, but when do we install user extensions?
There was a problem hiding this comment.
We install them only when they are added.
| extensionDiscovery.init(); | ||
| windowManager = WindowManager.getInstance<WindowManager>(proxyPort); | ||
|
|
||
| // call after windowManager to see splash earlier | ||
| try { | ||
| const extensions = await extensionDiscovery.load(); | ||
|
|
||
| // Start watching after bundled extensions are loaded | ||
| extensionDiscovery.watchExtensions(); |
There was a problem hiding this comment.
I don't see how this is broken, watchExtensions contains await this.whenLoaded; which is set at the end of load
There was a problem hiding this comment.
We can probably revert this, I did separate these just to understand the current flow better.
| // The path to the manifest file is the lens extension id | ||
| // Note that we need to use the symlinked path | ||
| const lensExtensionId = path.join(this.nodeModulesPath, extensionName, manifestFilename); | ||
| const lensExtensionId = extension.manifestPath; |
There was a problem hiding this comment.
Can we actually remove this variable and use extension.id?
There was a problem hiding this comment.
Yes if they are the same thing.. I just tried to keep this compatible with current logic.
There was a problem hiding this comment.
They should be the same. In the old implementation the id was created manually because we didn't have the extension object itself.
Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
|
|
||
| await this.installPackages(); | ||
| const extensions = bundledExtensions.concat(localExtensions); | ||
| const userExtensions = await this.loadFromFolder(this.localFolderPath); |
There was a problem hiding this comment.
Would it be good, in a future PR, to remove the dependence on this folder so that we can no longer use explicit symlinks and instead have a list of "dev" extensions to just require?
There was a problem hiding this comment.
Yes, that would be good improvement (but cannot be done in a patch release).
There was a problem hiding this comment.
okay sounds good. Maybe 4.1?
There was a problem hiding this comment.
4.1 is already quite loaded but we can try.
Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
* fix extension loader race conditions Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com> * cleanup Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com> * fix tests Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com> * fix remove Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com> * ensure symlinked (dev) extensions are installed on boot Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
Fixes following issues: