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

api on the fly - part 1 - generate forms from mutations #357

Merged
merged 9 commits into from
Jan 16, 2020

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Jan 9, 2020

Addresses the first three points of #339

This PR adds a temporary "mutations" view which lists available mutations and auto-generates forms for them on request.

This is orthogonal to cylc/cylc-flow#3469 which makes the mutations more declarative and brings them more inline with the Cylc CLI.

Getting the nested components and data model working correctly took nearly all the time. Thanks for the <component :is="class" /> suggestion @kinow, I would have wasted days otherwise 👍.

Overview:

  • Introspect the GraphQL schema for information about mutations.
  • Auto-generate forms to act as interfaces to these mutations.
  • When complete output JSON from these forms to be used as arguments in the GraphQL request.

Caveats:

  • Some of the default values in the GraphQL schema are presently set as string representations for list types. Either Graphene or GraphQL seems to convert all default types to JSON which is added as a string in the JSON response? This results in traceback and errors. 👀
  • Styling not yet complete.
  • The abstraction form generator is roughly independent of the component library being used but there is't a proper interface for implementing different libraries as yet. Not a big job but probably a bridge to cross if and when we get to it.

Justification:

There are some tools out there which could have been used, instead I wrote this from scratch. Just to dump my reasoning down here for the record:

There is a potential pathway where we convert GraphQL -> JSONSchema -> Web Form -> JSON -> GraphQL Arguments.

  • There are a lot of interfaces here and it gets a bit messy/
  • Most of the tools are immature
  • None of them seem to work with mutations properly (they are seem focused on the data model)
  • Most don't support Vue or are written for other frameworks.
  • None work with Vuetify.
  • Supporting GraphQL mutation schema natively seems more natural.

TODO:

  • Vuetify doesn't have an integer field, it has a "number" mode to the text field. This outputs strings rather than number which won't do.
  • Testing (basic).
  • Consider using an inheritance or composition model to keep things DRY.
  • Fix persistence of the FormGenerator model.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • No documentation update required.

@oliver-sanders oliver-sanders added this to the 0.3 milestone Jan 9, 2020
@oliver-sanders oliver-sanders self-assigned this Jan 9, 2020
@oliver-sanders oliver-sanders marked this pull request as ready for review January 9, 2020 21:57
@oliver-sanders
Copy link
Member Author

oliver-sanders commented Jan 9, 2020

Screenshot from 2020-01-10 10-59-08

Will output:

{
  "command": "\"put_external_trigger\"",
  "eventId": "1234",
  "eventMessage": "2345",
  "workflows": [
    "foo",
    "bar"
  ]
}

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

One or other typo I think, but found nothing wrong in the pull request. I think you have experienced pretty much every kind of problem I had with Vue.js 😄 Great work going through some of its limitations and working around its reactivity too. One step closer to mutations! 💥

src/views/Mutations.vue Outdated Show resolved Hide resolved
src/components/graphqlFormGenerator/FormGenerator.vue Outdated Show resolved Hide resolved
src/components/graphqlFormGenerator/FormGenerator.vue Outdated Show resolved Hide resolved
src/components/graphqlFormGenerator/FormGenerator.vue Outdated Show resolved Hide resolved
src/components/graphqlFormGenerator/FormGenerator.vue Outdated Show resolved Hide resolved
src/components/graphqlFormGenerator/FormInput.vue Outdated Show resolved Hide resolved
src/components/graphqlFormGenerator/FormInput.vue Outdated Show resolved Hide resolved
set (val) {
this.$emit('input', val)
}
},
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I bet it was not really fun to debug why the v-model was not working here ( I thought it would work, or :value.sync, which I tried but also didn't work 👀 )

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the data model took much longer than all the rest of the work put together. Really confused the hell out of me!

@oliver-sanders
Copy link
Member Author

I think you have experienced pretty much every kind of problem I had with Vue.js 

Yes, this was a pretty steep learning curve. I think I understand Vue a lot better now though.

@oliver-sanders
Copy link
Member Author

@kinow sneaking in one more commit to use mixins which make the code much D[n]RYer.

@oliver-sanders
Copy link
Member Author

@wxtim I've put you down for second review. This PR should work with the master branches of Cylc Flow and Cylc UIServer.

For second review I reckon:

  • Functional test.
  • Skim of code.
  • Check requirements/todos.

To test:

  • Cylc Flow: no suite needed.
  • Cylc UIS: launch as usual.
  • Cylc UI: build as usual.
  • Browser: Navigate to #/mutations.
  • Note some errors are to be expected (see caveats in the PR description).

The code is a little funky so some pointers here:

  • <component is="Foo" attr="value"> is effectively another way of writing <Foo attr="value">
  • Mixins are a way of sharing configuration between components via composition rather than inheritance (similar to Cylc families) https://vuejs.org/v2/guide/mixins.html
  • Components with names starting with a V come from Vuetify.

@wxtim
Copy link
Member

wxtim commented Jan 15, 2020

I think you have experienced pretty much every kind of problem I had with Vue.js 

Yes, this was a pretty steep learning curve. I think I understand Vue a lot better now though.

You can explain it to me now.

@wxtim
Copy link
Member

wxtim commented Jan 15, 2020

Am I supposed to do anything with the object created by using this?

Can I do something with this, or should something happen? Should I paste it into the console?

{
  "command": "\"nudge\"",
  "workflows": [
    "simple"
  ]
}

@wxtim
Copy link
Member

wxtim commented Jan 15, 2020

I have:

  • Read the code and spot any really obvious issues - at this rate my ability to read JS might overtake my abilities to read Fortran!
  • Tried running the UI - looks ok - I was able to nav to #mutations.

As far as I can tell this pull request does what @oliver-sanders intends it to do, and doesn't do other stuff. I've approved it, but I'd quite like answers to my ignorant questions. I'll merge if @oliver-sanders doesn't.

@oliver-sanders
Copy link
Member Author

Am I supposed to do anything with the object created by using this?

The JSON you see at the bottom of the form is just showing the data in the form. It should change as you type. The mutations page is a temporary view whilst we get all the form generation stuff sorted, the JSON is for development purposes.

Once we are finished that JSON will get attached to a GraphQL mutation and sent to the Workflow Server (via the UI Server).

@oliver-sanders
Copy link
Member Author

I'd quite like answers to my ignorant questions

Keep asking, reviews are a great way of learning and even prompting authors to think more about what they've done.

@wxtim
Copy link
Member

wxtim commented Jan 16, 2020

I'm happy that you've answered all my questions so I'm merging! Happy days.

@wxtim wxtim merged commit e06ef5a into cylc:master Jan 16, 2020
@oliver-sanders oliver-sanders deleted the api-on-the-fly branch January 16, 2020 20:00
@hjoliver hjoliver modified the milestones: 0.3, 0.2 Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants