fix(plugins): refactor instantiatePlugin from preproprocessor #3628
Conversation
|
✅ Build karma 2874 completed (commit d2d38a109d by @johnjbarton) |
|
✅ Build karma 477 completed (commit d2d38a109d by @johnjbarton) |
|
✅ Build karma 476 completed (commit d2d38a109d by @johnjbarton) |
|
I need a unit test for the new function. |
| const pluginInstances = new Map() | ||
|
|
||
| function createInstantiatePlugin (injector) { | ||
| const emitter = injector.get('emitter') |
There was a problem hiding this comment.
Why don't you inject the emitter directly?
There was a problem hiding this comment.
This is existing code. I don't understand your comment.
There was a problem hiding this comment.
It is true that it is existing code. I just wonder why emitter is not directly injected and instead injector is injected, from which we get emitter.
There was a problem hiding this comment.
My new answer: because we need the injector for the get operation.
lib/plugin.js
Outdated
| } | ||
| } catch (e) { | ||
| if (e.message.includes(`No provider for "${kind}:${name}"`)) { | ||
| log.error(`Can not load "${name}", it is not registered!\n Perhaps you are missing some plugin?`) |
lib/plugin.js
Outdated
| if (e.message.includes(`No provider for "${kind}:${name}"`)) { | ||
| log.error(`Can not load "${name}", it is not registered!\n Perhaps you are missing some plugin?`) | ||
| } else { | ||
| log.error(`Can not load "${name}"!\n ` + e.stack) |
lib/plugin.js
Outdated
| @@ -50,4 +50,36 @@ function resolve (plugins, emitter) { | |||
| return modules | |||
| } | |||
|
|
|||
| exports.resolve = resolve | |||
| const pluginInstances = new Map() | |||
There was a problem hiding this comment.
Why did you take this variable outside the factory function?
lib/plugin.js
Outdated
| const pluginInstances = new Map() | ||
|
|
||
| function createInstantiatePlugin (injector) { | ||
| const emitter = injector.get('emitter') | ||
| return function instantiatePlugin (kind, name) { | ||
| if (pluginInstances.has(name)) { | ||
| return pluginInstances.get(name) | ||
| } | ||
|
|
||
| let p | ||
| try { | ||
| p = injector.get(`${kind}:${name}`) | ||
| if (!p) { | ||
| log.error(`Failed to instantiate ${kind} ${name}`) | ||
| emitter.emit('load_error', `${kind}`, name) | ||
| } | ||
| } catch (e) { | ||
| if (e.message.includes(`No provider for "${kind}:${name}"`)) { | ||
| log.error(`Can not load "${name}", it is not registered!\n Perhaps you are missing some plugin?`) | ||
| } else { | ||
| log.error(`Can not load "${name}"!\n ` + e.stack) | ||
| } | ||
| emitter.emit('load_error', `${kind}`, name) | ||
| } | ||
|
|
||
| pluginInstances.set(name, p) | ||
| return p | ||
| } | ||
| } |
There was a problem hiding this comment.
Tell me if I am wrong, but this seems like dependency injection and we already have a module for that.
There was a problem hiding this comment.
There was a problem hiding this comment.
I agree that the cache is completely redundant and I will remove that part. The rest is error handling which could be in di but is not yet.
There was a problem hiding this comment.
I wonder if the cache was there to prevent multiple attempts to instantiate a missing preprocessor and consequently the same error being logged a lot of times 🤔
|
✅ Build karma 485 completed (commit 1665e403a3 by @johnjbarton) |
|
✅ Build karma 2882 completed (commit 1665e403a3 by @johnjbarton) |
|
✅ Build karma 484 completed (commit 1665e403a3 by @johnjbarton) |
johnjbarton
force-pushed
the
instantitate-plugins
branch
from
0bc3a18
to
c8ab319
Compare
|
✅ Build karma 2884 completed (commit 97e61bbf34 by @johnjbarton) |
|
✅ Build karma 487 completed (commit 97e61bbf34 by @johnjbarton) |
|
✅ Build karma 486 completed (commit 97e61bbf34 by @johnjbarton) |
|
✅ Build karma 2891 completed (commit 3f155edc5a by @johnjbarton) |
|
✅ Build karma 494 completed (commit 3f155edc5a by @johnjbarton) |
|
✅ Build karma 493 completed (commit 3f155edc5a by @johnjbarton) |
## [6.0.3](v6.0.2...v6.0.3) (2021-01-27) ### Bug Fixes * **plugins:** refactor instantiatePlugin from preproprocessor ([#3628](#3628)) ([e02858a](e02858a))
|
🎉 This PR is included in version 6.0.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
…runner#3628) add unit test for plugin.js add comments on role of function and cache
## [6.0.3](karma-runner/karma@v6.0.2...v6.0.3) (2021-01-27) ### Bug Fixes * **plugins:** refactor instantiatePlugin from preproprocessor ([karma-runner#3628](karma-runner#3628)) ([e02858a](karma-runner@e02858a))
No description provided.