-
Notifications
You must be signed in to change notification settings - Fork 8
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
SH4LIN
wants to merge
5
commits into
rtCamp:develop
Choose a base branch
from
SH4LIN:refactor/move-codegen-config-schema-to-base-config
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
9ebb249
Refactor: Move the schema file logic from individual codegen configur…
SH4LIN 2c7b1c7
Merge branch 'develop' into refactor/move-codegen-config-schema-to-ba…
justlevine eb3ea90
refactor: remove withCodegenConfig HOC and moved back to baseConfig()…
SH4LIN 1a2689f
Merge branch 'develop' into refactor/move-codegen-config-schema-to-ba…
SH4LIN 0a4b87d
refactor: renamed baseConfig() function to getBaseConfig()
SH4LIN File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,14 @@ | ||
import type { CodegenConfig } from '@graphql-codegen/cli'; | ||
import { sync as globSync } from 'glob'; | ||
import baseConfig from '@snapwp/codegen-config'; | ||
import { generateGraphqlUrl } from '@snapwp/core'; | ||
import { getBaseConfig } from '@snapwp/codegen-config'; | ||
import 'dotenv/config'; | ||
|
||
const GRAPHQL_GLOB = './src/**/*.graphql'; | ||
const graphqlFiles = globSync( GRAPHQL_GLOB ); | ||
|
||
const config: CodegenConfig = { | ||
...( graphqlFiles.length > 0 && { documents: GRAPHQL_GLOB } ), | ||
...baseConfig, | ||
// Use the schema file if it's set by CI. | ||
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 }`, | ||
}, | ||
}, | ||
}, | ||
], | ||
}; | ||
const config = getBaseConfig(); | ||
|
||
if ( graphqlFiles.length > 0 ) { | ||
config.documents = GRAPHQL_GLOB; | ||
} | ||
|
||
export default config; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,43 @@ | ||
import type { CodegenConfig } from '@graphql-codegen/cli'; | ||
import { generateGraphqlUrl } from '@snapwp/core'; | ||
justlevine marked this conversation as resolved.
Show resolved
Hide resolved
justlevine marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const baseConfig: CodegenConfig = { | ||
overwrite: true, | ||
generates: { | ||
'src/__generated/': { | ||
preset: 'client', | ||
plugins: [], | ||
config: { | ||
enumsAsTypes: true, | ||
skipTypename: true, | ||
useTypeImports: true, | ||
/** | ||
* Base configuration for GraphQL Codegen. | ||
* | ||
* @return The base configuration. | ||
*/ | ||
const getBaseConfig = (): CodegenConfig => { | ||
return { | ||
overwrite: true, | ||
generates: { | ||
'src/__generated/': { | ||
preset: 'client', | ||
plugins: [], | ||
config: { | ||
enumsAsTypes: true, | ||
skipTypename: true, | ||
useTypeImports: true, | ||
}, | ||
}, | ||
}, | ||
}, | ||
// Use the schema file if it's set by CI. | ||
// eslint-disable-next-line n/no-process-env | ||
schema: process.env.GRAPHQL_SCHEMA_FILE ?? [ | ||
{ | ||
[ generateGraphqlUrl( | ||
// eslint-disable-next-line n/no-process-env | ||
process.env.NEXT_PUBLIC_WORDPRESS_URL, | ||
// eslint-disable-next-line n/no-process-env | ||
process.env.NEXT_PUBLIC_GRAPHQL_ENDPOINT | ||
) ]: { | ||
headers: { | ||
// eslint-disable-next-line n/no-process-env | ||
Authorization: `${ process.env.INTROSPECTION_TOKEN }`, | ||
}, | ||
}, | ||
}, | ||
], | ||
}; | ||
}; | ||
|
||
export default baseConfig; | ||
export { getBaseConfig }; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,26 +1,9 @@ | ||
import type { CodegenConfig } from '@graphql-codegen/cli'; | ||
import * as dotenv from 'dotenv'; | ||
import baseConfigs from '@snapwp/codegen-config'; | ||
import { generateGraphqlUrl } from '@snapwp/core'; | ||
import { getBaseConfig } from '@snapwp/codegen-config'; | ||
|
||
dotenv.config( { path: '../../.env' } ); | ||
|
||
const config: CodegenConfig = { | ||
...baseConfigs, | ||
documents: './src/**/*.graphql', | ||
// Use the schema file if it's set by CI. | ||
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 }`, | ||
}, | ||
}, | ||
}, | ||
], | ||
}; | ||
const config = getBaseConfig(); | ||
config.documents = './src/**/*.graphql'; | ||
|
||
export default config; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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).There was a problem hiding this comment.
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
functionThere was a problem hiding this comment.
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...
There was a problem hiding this comment.
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 theschema
property as our default and only change theheader
.There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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