Skip to content

Bugfix/Feature: pascalCase; Bugfix: recursive nesting #9

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

Merged
merged 4 commits into from
Apr 5, 2020

Conversation

bohdanbirdie
Copy link
Contributor

This PR is going to fix the issue with pascalCase error, initially committed by @keesvanlierop
Also, I extended that functionality to support upper case enums as well.

The other thing that was failing for me was a recursive nesting.
Check this schema

type User {
  id: Int!
  username: String!
  tasks: [Task!]!
}

type Task {
  id: Int!
  description: String
  author: User!
  status: TaskStatus!
}

so, when I create a mock of a task -> user mock will be created, but then user mock will create a task mock and this will run recursively until the stack can't hold it anymore.
Overrides won't help because they come after the mock is created so I switched to the getters, so they won't create a nested mock if there is an override.

@bohdanbirdie
Copy link
Contributor Author

ok, @ardeois should be good for the review 👍

@ardeois ardeois self-requested a review April 3, 2020 19:57
Copy link
Owner

@ardeois ardeois left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @bohdanbirdie !
I have a few comments before we can merge this.
Especially on the default behaviour that now becomes upper-case instead of pascal-case
The rest is mostly conceptual

Also note that I'm working on a PR to update all dependencies, so don't worry about updating package versions

README.md Outdated
};
return {
get id() { return overrides && 'id' in overrides ? overrides.id! : '0550ff93-dd31-49b4-8c38-ff1cb68bdc38'},
get url() { return overrides && 'url' in overrides ? overrides.url! : 'aliquid'},
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a reason why you use a getter function?
It seems easier to do:

return {
   id: overrides?.hasOwnProperty('id') ? overrides?.id : '0550ff93-dd31-49b4-8c38-ff1cb68bdc38',
   url: overrides?.hasOwnProperty('url') ? overrides?.url : 'aliquid',
}

What do you think ?

package.json Outdated
@@ -43,7 +44,7 @@
"eslint-plugin-import": "^2.17.2",
"eslint-plugin-jest": "^22.20.0",
"eslint-plugin-prettier": "^3.0.1",
"graphql": "^14.5.8",
"graphql": "^14.6.0",
Copy link
Owner

Choose a reason for hiding this comment

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

FYI I'm working on a PR to update all dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you like to to rever it?
I think I had issues with different versions of graphql so had to update

Copy link
Owner

Choose a reason for hiding this comment

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

I just merged the update dependencies PR, so if you update your branch you should be good

src/index.ts Outdated
case 'NonNullType':
return generateMockValue(typeName, fieldName, types, currentType.type);
return generateMockValue(typeName, fieldName, types, currentType.type, enumValues);
Copy link
Owner

Choose a reason for hiding this comment

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

in getNamedType the param enumValues is before currenType but in generateMockValue it's after
Can you place the enumValues param at the same place for both functions (I don't really mind of it's before or after though)

src/index.ts Outdated

return ` ${fieldName}: ${value},`;
return ` get ${fieldName}() { return overrides && '${fieldName}' in overrides ? overrides.${fieldName}! : ${value}},`;
Copy link
Owner

Choose a reason for hiding this comment

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

like suggested, maybe just do:

return `        ${fieldName}: overrides?.hasOwnProperty('${fieldName}') ? overrides?.${fieldName} : ${value},`;

src/index.ts Outdated
enumValues,
);

return ` get ${field.name.value}() { return overrides && '${field.name.value}' in overrides ? overrides.${field.name.value}! : ${value}},`;
Copy link
Owner

Choose a reason for hiding this comment

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

same here

@ardeois ardeois added the minor Increment the minor version when merged label Apr 3, 2020
@ardeois
Copy link
Owner

ardeois commented Apr 3, 2020

@bohdanbirdie You should be good to update your branch with master.
I've updated all dependencies and added precommit hook that should run prettier for you. So it will format the README.md properly

@bohdanbirdie
Copy link
Contributor Author

@ardeois ok, updated everything, synced, added test and changed to pascal case as thee default

Copy link
Owner

@ardeois ardeois left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

@ardeois ardeois merged commit 120911c into ardeois:master Apr 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants