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

fix(plugins): refactor instantiatePlugin from preproprocessor #3628

Merged
merged 3 commits into from
Jan 27, 2021

Conversation

johnjbarton
Copy link
Contributor

No description provided.

@AppVeyorBot
Copy link

Build karma 2874 completed (commit d2d38a109d by @johnjbarton)

@karmarunnerbot
Copy link
Member

Build karma 477 completed (commit d2d38a109d by @johnjbarton)

@karmarunnerbot
Copy link
Member

Build karma 476 completed (commit d2d38a109d by @johnjbarton)

@johnjbarton
Copy link
Contributor Author

I need a unit test for the new function.

const pluginInstances = new Map()

function createInstantiatePlugin (injector) {
const emitter = injector.get('emitter')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you inject the emitter directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is existing code. I don't understand your comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: remove space: Cannot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

lib/plugin.js Outdated
@@ -50,4 +50,36 @@ function resolve (plugins, emitter) {
return modules
}

exports.resolve = resolve
const pluginInstances = new Map()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you take this variable outside the factory function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

lib/plugin.js Outdated
Comment on lines 53 to 81
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
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tell me if I am wrong, but this seems like dependency injection and we already have a module for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which module?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

lib/server.js Show resolved Hide resolved
@karmarunnerbot
Copy link
Member

Build karma 485 completed (commit 1665e403a3 by @johnjbarton)

@AppVeyorBot
Copy link

Build karma 2882 completed (commit 1665e403a3 by @johnjbarton)

@karmarunnerbot
Copy link
Member

Build karma 484 completed (commit 1665e403a3 by @johnjbarton)

@AppVeyorBot
Copy link

Build karma 2884 completed (commit 97e61bbf34 by @johnjbarton)

@karmarunnerbot
Copy link
Member

Build karma 487 completed (commit 97e61bbf34 by @johnjbarton)

@karmarunnerbot
Copy link
Member

Build karma 486 completed (commit 97e61bbf34 by @johnjbarton)

@AppVeyorBot
Copy link

Build karma 2891 completed (commit 3f155edc5a by @johnjbarton)

@karmarunnerbot
Copy link
Member

Build karma 494 completed (commit 3f155edc5a by @johnjbarton)

@karmarunnerbot
Copy link
Member

Build karma 493 completed (commit 3f155edc5a by @johnjbarton)

@johnjbarton johnjbarton merged commit e02858a into karma-runner:master Jan 27, 2021
karmarunnerbot pushed a commit that referenced this pull request Jan 27, 2021
## [6.0.3](v6.0.2...v6.0.3) (2021-01-27)

### Bug Fixes

* **plugins:** refactor instantiatePlugin from preproprocessor ([#3628](#3628)) ([e02858a](e02858a))
@karmarunnerbot
Copy link
Member

🎉 This PR is included in version 6.0.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

This was referenced Mar 13, 2021
anthony-redFox pushed a commit to anthony-redFox/karma that referenced this pull request May 16, 2023
…runner#3628)

add unit test for plugin.js
add comments on role of function and cache
anthony-redFox pushed a commit to anthony-redFox/karma that referenced this pull request May 16, 2023
## [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))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants