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

refactor: codegen config schema property moved to the codegen config package. #45

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

SH4LIN
Copy link
Collaborator

@SH4LIN SH4LIN commented Feb 6, 2025

What

  • This PR will refactor the common codegen config code to the @snapwp/codegen-config package.

Why

  • This refactoring is to avoid code duplication and keeping the minimal code on the userland

Related Issue(s):

How

  • Added withCodegenConfig function which will extend the base config with the custom configs provided from the individual codegen.ts file.

Checklist

  • I have read the Contribution Guidelines.
  • My code is tested to the best of my abilities.
  • My code passes all lints (ESLint, tsc, prettier etc.).
  • My code has detailed inline documentation.
  • I have added unit tests to verify the code works as intended.
  • I have updated the project documentation accordingly.

…ations to the base configuration. Add the `withCodegenConfig` function to extend the base configuration with the provided configurations.
@SH4LIN SH4LIN self-assigned this Feb 6, 2025
};

export default config;
export default withCodegenConfig( config );
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does a wrapper function give us that object destructuring doesnt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are now moving the schema into our codegen package. If we continue using only the object, it incorrectly retrieves process.env values. To resolve this, I converted it into a function called baseConfig(), which first loads the process variables from individual .env files. After that, it calls the function, ensuring we get the correct process.env values.

For the withCodegenConfig function, we are following the same approach as we did for withSnapWP. We take the config from the userland codegen.ts, give it higher priority, and add our common configs, such as HOC. This ensures that all extensibility functionality remains intact.

This HOC approach allows us to modify internal functionality and update functions without requiring users to migrate between versions.

For example, suppose we added the generateGraphqlUrl function in the userland codegen.ts and released the Next.js starter. Developers start using it, but later, we decide to change this function and introduce a new architecture. In that case, we would need to create a migration guide instructing users to adopt the new approach—despite them having no prior knowledge of the change. Additionally, we would have to follow a deprecation process for something that is purely an internal functionality.

Comment on lines -14 to -25
schema: process.env.GRAPHQL_SCHEMA_FILE ?? [
{
[ generateGraphqlUrl(
process.env.NEXT_PUBLIC_WORDPRESS_URL,
process.env.NEXT_PUBLIC_GRAPHQL_ENDPOINT
) ]: {
headers: {
Authorization: `${ process.env.INTROSPECTION_TOKEN }`,
},
},
},
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like that we've removed the entire schema.

There are many reasons that a project will need to change this, e.g. to change/add headers. (it's also one of the reasons I think the generateGraphQLUrl function should be removed).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is still an option to override this by passing it as the config in withCodegenConfig function

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, but a user would need to override the entire property...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you give me an example of what you mean by an entire property?

If you say they cannot override the schema property completely, they can. However, they won't be able to keep the schema property as our default and only change the header.

Copy link
Collaborator

Choose a reason for hiding this comment

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

#45 (comment)

If I needed to set the origin header in my local project, I would need to:

  • navigate the monorepo to find the defaults.
  • copy the defaults into my local codegen config
  • add my origin header.
  • actively monitor or changes to the default codegen config.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still don't like this. I feel like there is a difference between defaults and abstractions, and we really don't want to be creating an abstraction.

Will defer to @ayushnirwal

Copy link
Collaborator

@justlevine justlevine left a comment

Choose a reason for hiding this comment

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

Need some clarification on the approach 🙇

@SH4LIN SH4LIN requested a review from justlevine February 7, 2025 07:28
@SH4LIN
Copy link
Collaborator Author

SH4LIN commented Feb 11, 2025

@justlevine I have made changes to the code please re-review it, meanwhile I'll resolve merge conflict

Copy link
Collaborator

@justlevine justlevine left a comment

Choose a reason for hiding this comment

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

Some nitpicks, but then I think I need to defer to @ayushnirwal . I feel like we're adding tech-debt here without gaining anything.

Might help to see it with the merge-conflict resolved, since that's where the first pass of codegen types start getting added.

Comment on lines -14 to -25
schema: process.env.GRAPHQL_SCHEMA_FILE ?? [
{
[ generateGraphqlUrl(
process.env.NEXT_PUBLIC_WORDPRESS_URL,
process.env.NEXT_PUBLIC_GRAPHQL_ENDPOINT
) ]: {
headers: {
Authorization: `${ process.env.INTROSPECTION_TOKEN }`,
},
},
},
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still don't like this. I feel like there is a difference between defaults and abstractions, and we really don't want to be creating an abstraction.

Will defer to @ayushnirwal

packages/codegen-config/src/index.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@ayushnirwal ayushnirwal left a comment

Choose a reason for hiding this comment

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

We are abstracting away the choice of a schema file or endpoint right now. This seems good enough solution to solve our problem of CI depending on a server.

The real solution is to manage this choice in the scaffold process or let the Users decide by exporting 2 configs one using schema file and other using an endpoint. That being said it is out of the scope of a pre-alpha release. Given the choice between a stability and ease of use. I would prefer to choose the later option now and move on.

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.

3 participants