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

Add support for Embedded Types #104

Merged
merged 6 commits into from
Mar 28, 2016
Merged

Add support for Embedded Types #104

merged 6 commits into from
Mar 28, 2016

Conversation

nodkz
Copy link
Contributor

@nodkz nodkz commented Mar 20, 2016

According to http://mongoosejs.com/docs/subdocs.html#single-embedded

export const childSchema = new Schema({ name: 'string' }, {graphqlTypeName: 'Child', _id: false});

export const parentSchema = new Schema({
  child: childSchema
});

export const Parent = mongoose.model('Parent', parentSchema);

Add ability to pass childSchema to GraphQL. Also was added graphqlTypeName for child schema, because it must have Type name.

Mutations not realized yet, but for such field types I add catcher field.type.mongooseEmbedded. (realized in 2da95bf)

@tothandras review needed, may be your suggest better solution for embedded documents.

@tothandras
Copy link
Contributor

@nodkz Thank you, I like it! Although I have started to split the project into a general adapter (that waits a graffiti model as input) and a mongoose specific one, I might wait to merge this until that's finished. :)

@nodkz
Copy link
Contributor Author

nodkz commented Mar 23, 2016

@tothandras finished with mutations. Add converter objectType to inputObjectType.
I didn't know you preference for file naming, so called it by kebab-case: src/type/custom/to-input-object.js.

Splitting, hmm, will waiting new major version ;)
Also it will be great if you move graffiti specific configs from mongoose models under one root-name, e.g. graffiti.

const UserSchema = new mongoose.Schema({
  name: {
    type: String,
    graffiti: { // <-------------------------------------------------------------------
      hooks: {
        pre: (next, root, args, {rootValue}) => {
          const {request} = rootValue;
          authorize(request);
          next();
        },
        post: [
          (next, name) => next(`${name} first hook`),
          (next, name) => next(`${name} & second hook`)
        ]
      }
    }
  }
});

hooks may be used by native mongoose codebase in future.

@nodkz
Copy link
Contributor Author

nodkz commented Mar 23, 2016

@jashmenn your todo was resolved.
See 6d02faa and 2da95bf

This was referenced Mar 23, 2016
* @return {Object}
*/
function listToGraphQLEnumType(list, name) {
const values = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

reduce?

@tothandras
Copy link
Contributor

Let me merge and release this, the splitting will take more time. Can you squash the commits and rebase on master? And please use the angular commit message convention. Thank you! :)

@nodkz
Copy link
Contributor Author

nodkz commented Mar 23, 2016

First squash for me with merging.
I do it right now.

@nodkz
Copy link
Contributor Author

nodkz commented Mar 23, 2016

For me reduce is bulky ;p
If it's ok, I'm ready for squashing.

@@ -84,8 +84,10 @@ function stringToGraphQLType(type) {
* @return {Object}
*/
function listToGraphQLEnumType(list, name) {
const values = {};
list.forEach((val) => { values[val] = {value: val}; });
const values = reduce(list, (values, val) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

const values = list.reduce((allValues, value) => ({
    ...allValues,
    value
}), {});

? :) (didn't run it, may fail)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works on my project.
I do it like in getOrderByType method, which uses lodash reduce.
May rewrite on regular ES6 reduce.

PS. a subtle hint for tests :P

Copy link
Contributor

Choose a reason for hiding this comment

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

:P don't worry about them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working version might look like:

const values = list.reduce((allValues, value) => ({
    ...allValues,
    [value]: { value }
  }), {});

Some syntax-sugar-shit ))))))

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, right :) 👍

@tothandras
Copy link
Contributor

Also, I will write some tests then, don't worry about that. :)

@nodkz
Copy link
Contributor Author

nodkz commented Mar 23, 2016

Done.

enums and embedded schemas touched model, so use it for scope name in commit messages.
If need something to change - let me know.

@nodkz
Copy link
Contributor Author

nodkz commented Mar 24, 2016

Ups, I didn't notice conflicts yesterday with your master.
Fix this via rebase.

@tothandras tothandras merged commit 44356a3 into RisingStack:master Mar 28, 2016
@tothandras
Copy link
Contributor

@nodkz Thank you!

@tothandras
Copy link
Contributor

@nodkz The tests are failing, both locally and on CI. Can you take a look? I will do the same.

@nodkz
Copy link
Contributor Author

nodkz commented Mar 28, 2016

Yep. After 30 minutes.
28 марта 2016 г. 1:47 PM пользователь "András Tóth" <
notifications@github.com> написал:

@nodkz https://github.com/nodkz The tests are failing, both locally and
on CI. Can you take a look?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#104 (comment)

@tothandras
Copy link
Contributor

you don't need to rush :) it's okay later, take some rest

On Mon, Mar 28, 2016, 09:49 Pavel Chertorogov notifications@github.com
wrote:

Yep. After 30 minutes.
28 марта 2016 г. 1:47 PM пользователь "András Tóth" <
notifications@github.com> написал:

@nodkz https://github.com/nodkz The tests are failing, both locally
and
on CI. Can you take a look?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
<
#104 (comment)


You are receiving this because you modified the open/close state.

Reply to this email directly or view it on GitHub
#104 (comment)

@nodkz
Copy link
Contributor Author

nodkz commented Mar 28, 2016

No problem. I have now gone for lunch. Fixing tests after lunch is the
better pastime.
28 марта 2016 г. 1:51 PM пользователь "András Tóth" <
notifications@github.com> написал:

you don't need to rush :) it's okay later, take some rest

On Mon, Mar 28, 2016, 09:49 Pavel Chertorogov notifications@github.com
wrote:

Yep. After 30 minutes.
28 марта 2016 г. 1:47 PM пользователь "András Tóth" <
notifications@github.com> написал:

@nodkz https://github.com/nodkz The tests are failing, both locally
and
on CI. Can you take a look?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
<

#104 (comment)


You are receiving this because you modified the open/close state.

Reply to this email directly or view it on GitHub
<
#104 (comment)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#104 (comment)

@Secretmapper
Copy link

Secretmapper commented Apr 29, 2016

Are mutations already supported @nodkz ? I've defined a subdocument array but it is not present in the schema input and hence I can't update/add a subdocument:

My schema:

const MaterialSchema = new Schema({
  title: { type: String },
  description: { type: String },
  chapters: [{
    title: String,
    content: [Schema.Types.Mixed]
  }],
  createdAt: {
    type: Date,
    default: Date.now()
  }
})

.graphql input is missing 'chapters'

input updateMaterialInput {
  title: String
  description: String
  createdAt: Date
  id: ID!
  clientMutationId: String!
}

Hoping I just missed something :)

@nodkz
Copy link
Contributor Author

nodkz commented Apr 29, 2016

First of all a see problem with Schema.Types.Mixed. I can not imagine how it can be passed via graphql (it's strongly typed) so such magic must be stringified between mongoose and graphql resolver. So type mixed is unsupported thing.

Why it not convert array of chapters to input, i can not imagine. Right now I on mobile, so can see and try help only on Thursday. Right now I recommend you try sub-docs (http://mongoosejs.com/docs/subdocs.html) for chapters.

Also I have many hacks on graffiti, and Internally I totally rewrite it in own project from scratch. And right now it under heavy api changing. I'm trying make comfortable and heavy customizable thing. I think it will be open sourced at end of may, when api will be stable and covered all our needs.

@Secretmapper
Copy link

Secretmapper commented Apr 30, 2016

@nodkz Actually thinking about it, you are right Mixed is impossible since graphql has to be strongly typed.

For some reason though array of chapters doesn't seem to be added to Input for some reason, even if using subdocs. Have done some digging through the source and tried to take a stab on it but still doesn't seem to get picked up (though it appears in changedMaterialNode weirdly enough)

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