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.

Sign up for GitHub

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

Jump to bottom

Graphql codegen typescript effect #701

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Graphql codegen typescript effect #701

wants to merge 6 commits into from

Conversation

vecerek
Copy link
Contributor

@vecerek vecerek commented May 2, 2024

🚨 IMPORTANT: Please do not create a Pull Request without creating an issue first.

Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of
the pull request.

Description

Implements a new GraphQL codegen plugin that outputs a client SDK written using Effect.

Related # (issue)

#664

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as
    expected)
  • This change requires a documentation update

Screenshots/Sandbox (if appropriate/relevant):

Adding links to sandbox or providing screenshots can help us understand more about this PR and take
action on it as appropriate

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can
reproduce. Please also list any relevant details for your test configuration

  • Test A
  • Test B

Test Environment:

  • OS: MacOS 14.2.1 (23C71)
  • @graphql-codegen/...:
  • NodeJS: v20.11.0

Checklist:

  • I have followed the
    CONTRIBUTING doc and the
    style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Further comments

I've mostly followed the implementation of @graphql-codegen/typescript-graphql-request

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented May 2, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: f374095

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

This PR includes changesets to release 1 package
Name Type
@graphql-codegen/typescript-effect Minor

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

@vecerek vecerek mentioned this pull request May 2, 2024
Closed
14 tasks
@vecerek
Copy link
Contributor Author

vecerek commented May 2, 2024

@saihaj how do I get the ball rolling on this PR? I opened #665 about 2 months ago and nobody has looked at it since. This PR is the re-opened version of it after my fork sync mistake that closed it.

@vecerek vecerek force-pushed the graphql-codegen-typescript-effect branch from 3141c9a to aa10805 Compare May 2, 2024 06:00
@vecerek
Copy link
Contributor Author

vecerek commented May 2, 2024

@Urigo I've just removed the no longer necessary patch of @urql/introspection@0.3.2. The fix was added in v0.3.3. I think it should be ready for another attempt at running the CI. Lemme know if you want me to squash all the commits into a single one after all the tests pass 🙇

vecerek and others added 6 commits May 15, 2024 07:24
@vecerek
fix: add packageManager to the manifest
14585cb 
to let corepack take care of yarn version resolution,
so that the user does not have to install different
versions of yarn on their machine
@vecerek
feat: implement a graphql-codegen plugin that outputs a client SDK wr…
873debd 
…itten in Effect (dotansimha#664)
@vecerek
refactor(typescript-effect): remove the use of schema and move logic …
f5f3b2a 
…over to the client constructor
@vecerek @tim-smart
feat(typescript-effect): export a basic GraphQLClient layer from with…
b2ed063 
…in the generated SDK

Co-authored-by: Tim Smart <hello@timsmart.co>
@vecerek
feat(typescript-effect): make plugin output mode configurable
54e996e
@vecerek
fix: remove redundant patch of @urql/instrospection
f374095 
as a fixup of 3416b87
@vecerek
Copy link
Contributor Author

vecerek commented May 15, 2024

It's a mystery to me why I'm getting the following error during the build step:

Type 'import("/Users/avecerek/Code/graphql-code-generator-community/node_modules/@graphql-codegen/visitor-plugin-common/typings/utils").DeclarationBlock' is not assignable to type 'import("/Users/avecerek/Code/graphql-code-generator-community/packages/plugins/c-sharp/c-sharp/node_modules/@graphql-codegen/visitor-plugin-common/typings/utils").DeclarationBlock'.

This PR does not touch @graphql-codegen/visitor-plugin-common nor c-sharp packages.

@vecerek vecerek force-pushed the graphql-codegen-typescript-effect branch from 1821b0e to f374095 Compare May 15, 2024 05:57
@vecerek
Copy link
Contributor Author

vecerek commented May 15, 2024

The problem is introduced in 873debd. I have no clue why but yarn suddenly resolves the node_modules differently after a new package is added. I'm lost. Could someone more knowledgeable about the tooling used in this repo help me here?

RoryO
RoryO reviewed May 24, 2024
View reviewed changes
packages/plugins/typescript/effect/src/visitor.ts

export class GraphQLClient extends Context.Tag('GraphQLClient')<
GraphQLClient,
Http.client.Client<
Copy link

Choose a reason for hiding this comment

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

Shouldn't this be <GraphQLSuccessResponse, Http.error.HttpClientError | MissingDataGraphQLResponseError, never> now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't upgraded to the latest version of effect, yet. I'm first trying to figure out what's wrong with the current build. Yarn's dependency hoisting got messed up after I added this new workspace. I don't understand what's going on and how to work around the issue.

@vecerek vecerek mentioned this pull request Jun 1, 2024
Open
10 tasks
@vecerek
Copy link
Contributor Author

vecerek commented Jun 1, 2024 •
edited
Loading

My strategy to take this work further is to modernize the tooling first. I'd appreciate it if one of the maintainers could take a look at #735 🙇

@vecerek vecerek mentioned this pull request Jun 1, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RoryO RoryO RoryO left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vecerek @RoryO