eddeee888 / graphql-code-generator-plugins

List of GraphQL Code Generator plugins that complements the official plugins.
MIT License
44 stars 10 forks source link

fix: collocate resolver output files with schema.graphql #244

Closed jb-1980 closed 2 months ago

jb-1980 commented 3 months ago

Addresses #243

The issue suggests using a config option, but when I dug into the code I saw we could easily modify the moduleName to be a relative path to the module instead of just the basename. Using a relative path still creates the exact same behavior as the previous implementation, but also allows for deeper nesting of modules.

changeset-bot[bot] commented 3 months ago

🦋 Changeset detected

Latest commit: a50a671257f6f0e5ac1c41c5637ab85a6e188182

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | ---------------------------------------- | ----- | | @eddeee888/gcg-typescript-resolver-files | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

eddeee888 commented 3 months ago

Thanks for looking into this @jb-1980 !

I would be good to make sure existing tests pass by running e2e tests:

nx e2e-run typescript-resolver-files-e2e -c clean-run # Cleans up all generated files, generates files for each test.

And if all existing tests pass, maybe adding new test cases for nested scenarios would be good. I think test-scalars-module and test-modules-resolver-main-file-mode are good tests to extend in this case (but if you see more tests that could benefit, feel free to add more 🙂)

Here's an example of command to clean up and re-generate files for test-scalars-module:

nx e2e-cleanup typescript-resolver-files-e2e -c test-scalars-module \
& nx graphql-codegen typescript-resolver-files-e2e -c test-scalars-module
jb-1980 commented 3 months ago

@eddeee888 I have spent the morning practicing with the tests and I have a few questions about some implementation details.

The tests don't pass because the codegen is using the same schema for what arguably may be two modules. For example, in test-scalars-module the schema are defined in test-scalars-module/schema/**/*.graphql, but also used in test-scalars-module/schema-overrides.

I see this as the equivalent of "Create a resolver tree for Module B based on the schema in Module A". So I am left to surmise two possible reasons for this:

  1. It was easier to implement this way because you could write code for one test, and reuse it for a similar test, even though it would never actually be written this way in the wild.
  2. There is actually a use case for creating a resolver tree for Module B from the schema in Module A, and I just have yet to grok what that case should be.

If it is case 1, then I can duplicate the schemas in the necessary test folders (test-add-option, test-extended-object-types, test-whitelisted) similar to what I have done with test-scalars-module to get the tests to pass. However, if it is case 2, then I am kind of at a loss for how to create an outputDir path in a general sense because it will always be relative to the schema directory (Module A) which will break the paths in Module B.

eddeee888 commented 3 months ago

Hi @jb-1980 ! Thank you for implementing and looking through the tests 🙌 I agree with your approach to duplicate the schema files to separate the test cases. The shared schema file is the old setup and newer tests (e.g. this one )would use the schema option you use.

Also, apologies for the delayed, let me take a look at your implementation 🙂

eddeee888 commented 3 months ago

I've had a look at the implementation, looks great @jb-1980! Thank you! Based on your implementation, I saw a few bad test setups which I've fixed separately here: https://github.com/eddeee888/graphql-code-generator-plugins/pull/246

I think if you rebase on the latest master, all the e2e tests should pass! Maybe you are already planning this, but if you have a chance, could you please add a few test case for nested schema files please? 🙏

Getting very close now!

jb-1980 commented 3 months ago

Thanks for cleaning that up @eddeee888. All the current tests now pass, and I added another to test the nested modules.

eddeee888 commented 3 months ago

Thank you! There are a few failing tests for Windows, let me see if I can fix it

eddeee888 commented 3 months ago

I've added a commit to make relativePathFromBaseToModule work for both Posix and Windows path here: https://github.com/eddeee888/graphql-code-generator-plugins/pull/244/commits/a50a671257f6f0e5ac1c41c5637ab85a6e188182

We needed this because given path.relative results in path/to/nested/dir on Linux/Mac but results in path\to\nested\dir. The added functionality turns the path to an array e.g. ["path", "to", "nested", "dir"] so we can use it to construct the paths later.

Let me know if you know a better way around it 🙂 I'll approve this PR so you have a chance to have a final review. Feel free to merge when you are happy with everything. Thank you for pushing this forward!

jb-1980 commented 2 months ago

I've added a commit to make relativePathFromBaseToModule work for both Posix and Windows path

Good catch. I was not thinking of Windows, so I am thankful you were able to catch that and fix it.

And thanks for creating this library. It has been such a great tool in handling a lot of the boiler plate of setting up a graphql server, letting me focus on the "hard" stuff of figuring out how to resolve data to the match the schema.

jb-1980 commented 2 months ago

Feel free to merge when you are happy with everything.

I don't have write access, so please merge when you have a chance.

eddeee888 commented 2 months ago

Merged! Thank you! It's me who should say thank you for using and fixing issues in the library 🕺