-
Notifications
You must be signed in to change notification settings - Fork 35
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
[WIP] GraphiQL editor and rework usages of MyQuery #81
base: master
Are you sure you want to change the base?
Conversation
I got the CI to pass, but I can't get circleci to pass. I think it needs to be told to use node 16 rather than node 12, like it is using now. |
Very cool, thanks @retrodaredevil! Your changes sound reasonable, thanks for the detailed descriptions. Give me some time to digest this... |
What version of Grafana are you developing and testing this with? Will we have to drop Grafana 7 support? |
I've been testing this only with Grafana 8, but I don't believe I have used any Grafana 8 specific features. I know the annotation react support stuff was added in Grafana 7.2, so I assume minimum required Grafana version is going to be 7.2. I bumped the Grafana version a few patches to make one of the CI checks pass, which solved a problem described here: grafana/grafana#38561 I can test on Grafana 7 sometime before merging. It might be a while depending on when graphql/graphiql#2291 gets merged. |
@retzkek I'm having trouble getting circle ci to build. I noticed that it's failing in master even right now. Is that something I shouldn't worry about fixing? I'm almost ready to mark this as non-draft, and I've been struggling to get it to work. I got it working for the most part, but it seems to fail around the same time of the current fail on the master branch (I think). I got it working to a point where it's not complaining about an old node version. |
Nah you could even remove circleci if you wanted, or just ignore it and we'll remove it later, we don't need both and it's not worth the trouble. |
Pretty much it's all done now, except for it working on any Grafana 7 version. Grafana 7 uses Webpack 4, which codemirror (graphiql depends on) does not like at all. I'm going to continue to try and get it working on Grafana 7 and see if I can incorporate a workaround for webpack/webpack#5429 and surmon-china/vue-codemirror#86 (although we don't use vue obviously). In my last commit, I changed the variable config editor to use a class. I tried getting that to work using the functional style, but IMO the functional style is hard to understand if we're trying to cache stuff. It's just easier to use a constructor. |
Finals and final projects for classes are keeping me busy. I'll get back to this eventually lol, but I did not have a good time trying to get it to work on Grafana 7. |
Grafana 9 is going to be released soon, so I'm ok dropping support for 7. Thanks for trying! |
Cool. That will probably end up happening. I just need to test this a bit more because I think there are some bugs in it. |
WIP because I'm waiting on graphiql to be updated.
This is all of the changes for getting GraphiQL support. I also went out of my way to rework how we were dealing with MyQuery and which defaults were being used.
.editorconfig
. TBH, I added this on accident, but I think it's useful, so if you want to keep it, awesome. If not, let me know and I'll remove it.render()
calls.render()
being called so frequently when changing other stuff, I "cache" the GraphiQL element in the annotation editor only. I might add this functionality to the other editors before merging.MyQuery
. For my own sanity, I wanted to get rid of as much legacy data as possible, and not save any unneeded data, since regular, variable, and annotation queries use different configurations.MyQuery
definition looks complicated, but that's the best I came up with to make all the common fields always present, and then make all the other disjoint fields possibly undefined.annotationTitle
,annotationText
,annotationTags
because Grafana actually provides some of that for us now, although it requires that it uses one of the fields already present, which can be convenient, but doesn't allow us to have custom substitutions. This is whereadditionalTexts
comes in. You can add as many custom fields as you want, which you can then select below for title, tags, text, or ID (I don't know what ID is used for).If you want any screenshots, let me know. There should be no breaking changes in this.
The
dist/
folder is updated with the latest changes, but since Grafana doesn't support unsigned datasources without dev mode enabled, I think we should consider making a decent.gitignore
and remove thedist/
folder altogether. That's another discussion, though.This has a lot of commits, so I would recommend you squashing before merging. Don't merge this yet, this is just in a state where you can look at the changes on your own time, @retzkek.