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

Document and enforce camelCase plugin id format #52190

Closed
mshustov opened this issue Dec 4, 2019 · 10 comments · Fixed by #53759
Closed

Document and enforce camelCase plugin id format #52190

mshustov opened this issue Dec 4, 2019 · 10 comments · Fixed by #53759
Assignees
Labels
discuss Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@mshustov
Copy link
Contributor

mshustov commented Dec 4, 2019

The legacy plugin system uses a convention that plugin id should be in snake_case. All public API provided via hapi plugin system followed this convention
In legacy platform plugin public contract looks like:

// plugins/my_plugin/index.js
new kibana.Plugin({
  id: 'my_plugin',
  require: ['another_plugin']
});
// later in code
server.plugins.another_plugin.get()

Although on the client side plugins can register an angular directive or export API under any name, usually in camelCase.

In New platform, we should formalize the requirement for id format, because it's the part of the plugin public contract.
Dependent plugins use it:

  • to declare a dependency on a plugin
"requiredPlugins": ["another_plugin"]
  • to access dependency API
setup(core, deps){
  deps.another_plugin.get() // can be passed deeper down the tree
  • interact with plugins
apps.navigateToApp('another_plugin')

Since id became a first-class citizen in the app codes, shouldn't we review our requirement and enforce the camelCase format for it?

We can add runtime check in discovery to enforce this rule.

@mshustov mshustov added discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Dec 4, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@stacey-gammon
Copy link
Contributor

+1 to camelCase!

@pgayvallet
Copy link
Contributor

If we choose to go with camel, should the plugin's folder follows this rule too?

@mshustov
Copy link
Contributor Author

mshustov commented Dec 5, 2019

@pgayvallet
Copy link
Contributor

+1 to camelCase then

deps.anotherPlugin.get() feels way better than deps.another_plugin.get()

@joshdover
Copy link
Contributor

+1 to camelCase as well

@joshdover
Copy link
Contributor

Going with camelCase. Need documentation and runtime validation of camelCase.

@joshdover joshdover changed the title [Discuss] Recommended plugin id format Document and enforce camelCase plugin id format Dec 17, 2019
@mshustov mshustov self-assigned this Dec 23, 2019
@mshustov
Copy link
Contributor Author

mshustov commented Dec 23, 2019

As we decided to go with #46705 the proposed change will create a mismatch between pluginId fooBar and configPath foo_bar. Now every plugin in camelCase has to define configPath explicitly.

// kibana.json
"configPath": ["foo_bar"],
"id": "fooBar"
// kibana.yml
foo_bar.baz: 42

If configPath not specified explicitly, the platform will use the pluginId to access the config.
Do we want to convert pluginId from camelCase to snake_case reading the config to enforce snake_case convention for the config values?. I'd rather avoid this, because it worsens the code predictability. That being said, this is okay:

// kibana.json
"id": "fooBar"
// kibana.yml
fooBar.baz: 42

Otherwise it will be

// kibana.json
"id": "fooBar"
// kibana.yml
foo_bar.baz: 42

@pgayvallet
Copy link
Contributor

pgayvallet commented Jan 6, 2020

As we want to go away from camelCase in configuration in #46705, I'm afraid letting fooBar.baz as the default for the fooBar plugin might not be something we want. I agree automatic conversion to foo_bar is not ideal, However this is still a predictable and bijective (at least I think for the bijection?) transformation, so that might be the way to go?

@joshdover
Copy link
Contributor

I think we should have good defaults here so that plugins don't have to specify configPath manually in most cases.

I don't think the risks of auto-converting the plugin ID to get the the default configPath are very high, but should definitely be documented well. I don't think we should auto-convert the configPath if it is explicitly set though.

// kibana,json
"id": "fooBar"
// kibana.yml
foo_bar.baz: 42

As an aside, it'd be nice if xpack plugins didn't have to specify a configPath manually, but that's a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants