-
Notifications
You must be signed in to change notification settings - Fork 32
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
Create Plugins: Fix plugin id mismatch #834
Conversation
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.
Nice cleanup, mostly looks good to me 👏
One thing though: unfortunately we also need to update the getTemplateData()
function in utils.templates.ts
to make sure that the update and generate flows are the same.
(@Ukochka's PR for refactoring the template-data generation should make these updates and fixes simpler in the future.)
@@ -25,7 +25,9 @@ export const normalizeId = (pluginName: string, orgName: string, type: PLUGIN_TY | |||
|
|||
const newPluginName = pluginName.replace(re, '').replace(nameRegex, ''); | |||
const newOrgName = orgName.replace(nameRegex, ''); | |||
return newOrgName.toLowerCase() + '-' + newPluginName.toLowerCase() + `-${type}`; | |||
const newType = type === PLUGIN_TYPES.scenes ? PLUGIN_TYPES.app : type; |
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.
Nice! (Maybe it would be nice in the future to define an explicit mapping between these two different "types", e.g. in the constants.)
@@ -2,7 +2,7 @@ | |||
"$schema": "https://raw.githubusercontent.com/grafana/grafana/main/docs/sources/developers/plugins/plugin.schema.json", | |||
"type": "app", | |||
"name": "{{ titleCase pluginName }}", | |||
"id": "{{ normalize_id pluginName orgName 'app' }}", | |||
"id": "{{ pluginId }}", |
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.
If pluginId
is now the source of truth for all templates probably best if we remove the handlebars normalize_id
helper registration in utils.handlesbars.ts
to prevent someone introducing its usage again in a template in the future.
@leventebalogh regarding your suggestion:
Do you mean doing something like: pluginId: normalizeId(pluginJson.name, pluginJson.org, pluginJson.type), ? If that's the case, couldn't it be an issue, since the plugin id will change? E.g. all the old |
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.
Sorry, I was wrong @oshirohugo. You are right, during updates the pluginId
should come from the plugin.json. Changes look good to me 👍
What this PR does / why we need it:
There was some mismatching in the pluginId created in the templates, this PR fix it.
Which issue(s) this PR fixes:
Fixes #758
Special notes for your reviewer:
To test this, I create projects using the old version and the new version for all plugin types: app, datasource, panel and scenesapp (with all the options on). I compared the results, and they were the same for app, datasource and panel. For scenesapp the fixes made all the pluginIds to be the same and the export path new has the
-app
sufix instead of-scenesapp
📦 Published PR as canary version:
Canary Versions
✨ Test out this PR locally via:
npm install @grafana/create-plugin@4.2.6-canary.834.966fae0.0 # or yarn add @grafana/create-plugin@4.2.6-canary.834.966fae0.0