-
Notifications
You must be signed in to change notification settings - Fork 450
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
feat(codegen): generate SanityQueries interface in @sanity/codegen #6997
feat(codegen): generate SanityQueries interface in @sanity/codegen #6997
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
@romeovs is attempting to deploy a commit to the Sanity Team on Vercel. A member of the Team first needs to authorize it. |
generateTypeNodeTypes( | ||
identifierName: string, | ||
typeNode: TypeNode, | ||
): {type: string; typeName: string} { |
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.
To reuse the types later (like when generating the type map), we need to be able to reference them and for that we need the actual type name that was generated.
This change makes generateTypeNodeTypes
return the generated names so we can reuse them.
This is a breaking change to the @sanity/codegen
package. I'm happy to create a new method for this if that makes more sense.
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.
Hm it would be good not to break this method.
A few alternatives:
- the
generateQueryMap
could also just use this private map to lookup the generated type name. - add a
addToQueryMap(name: string, type: string)
that collects all the queries generated in the class, thengenerateQueryMap()
can just traverse that list and spit out the types for all. - Expose the typeNameMap in a method, then we can extract the generated typeName separately.
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 agree that it's better to leave the signature for generateTypeNodeTypes
unchanged, but I made these changes because I wanted to avoid coupling.
Unless I am misunderstanding your suggestions, I think each of them introduce some coupling:
the
generateQueryMap
could also just use this private map to lookup the generated type name
This requires all the generateTypeNodeTypes
calls to happen before the call to generateQueryMap
, which might introduce subtle bugs.
add a
addToQueryMap(name: string, type: string)
that collects all the queries generated in the class, thengenerateQueryMap()
can just traverse that list and spit out the types for all.
This also requires all the generateTypeNodeTypes
calls to happen before the call to generateQueryMap
, which might introduce subtle bugs.
Expose the
typeNameMap
in a method, then we can extract the generated typeName separately.
The problem with the typeNameMap
is that it is not fully bijective.
If we call getTypeName
with the same desired name twice, the result will be different (and we cannot recover the first name because it is overwritten).
We could imagine a situation where the following arises (when a user has two queries named myQuery
for instance):
const schema = { ... }
const generator = new TypeGenerator(schema)
const typeA = { ... }
const typeB = { ... }
let code = ""
code += generator.generateTypeNodeTypes("MyQuery", typeA)
code += generator.generateTypeNodeTypes("MyQuery", typeB)
and this would generate two different identifiers, MyQuery
and `MyQuery_2.
In this case it is impossible to ask the TypeGenerator
"what is the type name used for MyQuery
?" since it has two answers.
Even worse, in the current implementation, the TypeGenerator
only stores the last type (ie. MyQuery_2
) and forgets about the first one altogether.
I have implemented your first proposal in the PR now, but wanted to bring this up.
Maybe a good follow up would be to refactor TypeGenerator
so there is less coupling?
067696c
to
3910dba
Compare
3910dba
to
7f60837
Compare
7f60837
to
1c819f5
Compare
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.
It's possible to add defineQuery
top-level without breaking CJS, I pushed a branch demoing it here: https://github.com/sanity-io/sanity/tree/bc-groq-define-query/packages/groq
The key is to:
- remove the
source
field, and add"type": "module"
topackage.json
, this disables the automatic bundling heuristics in@sanity/pkg-utils
:sanity/packages/groq/package.json
Lines 32 to 33 in 55aaf00
"require": "./lib/groq.cjs", "default": "./lib/groq.js" - Add
bundle
conditions topackage.config.ts
, this allows us to setup the ESM and CJS targets separately:sanity/packages/groq/package.config.ts
Lines 6 to 10 in 5801ef6
legacyExports: false, bundles: [ {source: './src/_exports.cts.ts', require: './lib/groq.cjs'}, {source: './src/_exports.mts.ts', import: './lib/groq.js'}, ], - With separate entrypoints we can handle the CJS target in a way that maintains backwards compatibility: https://github.com/sanity-io/sanity/blob/5801ef613a73c55e4418118e61cc1e0f2aa3ad61/packages/groq/src/_exports.cts.ts
- While the ESM version doesn't have to deal with any of this: https://github.com/sanity-io/sanity/blob/5801ef613a73c55e4418118e61cc1e0f2aa3ad61/packages/groq/src/_exports.mts.ts
- I've added two (1, 2) quick tests that demonstrates that it works:
git checkout bc-groq-define-query && pnpm install && cd packages/groq && pnpm turbo build && node test.cjs && node test.mjs
@stipsan That did the trick! Thanks for the input, I've updated the PR accordingly. Can you verify that this will be fully backwards-compatible? |
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.
Yep looks good 👍 great work 💖
6a6f327
to
6999fe2
Compare
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
… can store it for later use
6999fe2
to
86af3c9
Compare
@romeovs I'll be merging this to a feature branch on our end so we can properly run all the CI tests, and then PR to |
3b2329c
into
sanity-io:feat/define-groq-query-typemap
…6997) (#7304) * fix(deps): update dependency @sanity/client to ^6.21.1 (#7215) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * feat(typegen): groq/define module with a defineQuery helper * feat(typegen): Recognize queries that are wrapped in defineQuery calls * feat(typegen): Add overloadClientMethods option to @sanity/codegen * refactor(codegen): Return the typename of a generated type node so we can store it for later use * feat(typegen): Add generateTypeMap helper to TypeGenerator * feat(typegen): Allow codegen cli to generate the SanityQueries type map * docs(typegen): Add docs about defineQuery to groq README * refactor(groq): Allow defineQuery to live in the groq package * refactor(groq): Use the correct groq import in codegen tests * fix(codegen): Only use defineQuery when it comes from the groq package * refactor(codegen): Avoid changing the signature for generateTypeNodeTypes * refactor(codegen): Use typeNode to reference types in typeGenerator * feat(codegen): Handle duplicate query strings --------- Co-authored-by: Romeo Van Snick <[email protected]>
…6997) (#7304) * fix(deps): update dependency @sanity/client to ^6.21.1 (#7215) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * feat(typegen): groq/define module with a defineQuery helper * feat(typegen): Recognize queries that are wrapped in defineQuery calls * feat(typegen): Add overloadClientMethods option to @sanity/codegen * refactor(codegen): Return the typename of a generated type node so we can store it for later use * feat(typegen): Add generateTypeMap helper to TypeGenerator * feat(typegen): Allow codegen cli to generate the SanityQueries type map * docs(typegen): Add docs about defineQuery to groq README * refactor(groq): Allow defineQuery to live in the groq package * refactor(groq): Use the correct groq import in codegen tests * fix(codegen): Only use defineQuery when it comes from the groq package * refactor(codegen): Avoid changing the signature for generateTypeNodeTypes * refactor(codegen): Use typeNode to reference types in typeGenerator * feat(codegen): Handle duplicate query strings --------- Co-authored-by: Romeo Van Snick <[email protected]>
Description
This PR is one of two PR's that implements my feature request (#6934) in
sanity-io/sanity
, with the help of some pointers from @sgulseth.The companion PR that implement the corresponding
SanityQuery
in@sanity/client
lives at sanity-io/client#858.This PR implements a couple of things.
1. Add a
defineQuery
helper to thegroq
packageThis helper can be used instead of the
groq
template tag.This is necessary because TypeScript is not capable of strictly typing tagged template strings (yet, see microsoft/TypeScript#33304) and we need the resulting query to be typed as the literal string rather than
string
.For example, in the following snippet,
query
is typed as a genericstring
.While in the following code snippet,
query
is typed as"*[_type = 'foo']"
which is more specific.We will need this specificity later to type the result of
SanityClient.fetch
.Note: IdeallydefineQuery
would live in the top-levelgroq
package, but becausegroq
already has a default export (thegroq
template tag) this is not possible. As far as I can tell is impossible to safely mix default exports with named exports when transpiling to CommonJS without forcing the users of the (CommonJS version of the) library to change their setup. AddingdefineQuery
as a named export ofgroq
was breaking this assertion sincegroq
moved to the.default
property on the import.Instead, to preserve full backwards compatibility, I have putdefineQuery
ingroq/define
.Thanks to input from @stipsan, we can export
defineQuery
as a named export fromgroq
🙌2. Find
defineQuery
queries in@sanity/codegen
Allows
@sanity/codegen
to find and parse queries that are defined by the newdefineQuery
wrapper.groq
-tagged queries are still detected and used as before too.3. Generate
SanityQueries
interfaceAdd
generateTypeMap
toTypeGenerator
that generates the type augmentation for@sanity/client
'sSanityQueries
:This feature is currently disabled by default and can be enabled by setting
"overloadClientMethods": true
in'sanity-typegen.json
.4. Update
@sanity/cli
Update
@sanity/cli
to make use of the new utility function to generate the type map.What to review
codegen
command in@sanity/cli
and make sure the resulting generated code matches expecations.groq
vsgroq/define
question (see above). It might be possible to get somedefineQuery
ingroq
usingesModuleInterop
, but I'm not sure about the assumptions Sanity makes about the users of that package.pkg-utils
does not seem to support it currently.Testing
defineQuery
helper.codegen
cli action, but I'm happy to help set those up if required.Notes for release
There is a breaking change in the
@sanity/codegen
package:TypeGenerator.generateTypeNodeTypes
now returns{ type: string, typeName: string }
instead ofstring
. I figured that since@sanity/codegen
is still in beta, that change would be safe to make. I've updated all the call sites (only one) in the repo to match the change.I have left some inline notes too to clarify some decisions.
There is also a TS Playground that summarizes the changes made by this PR and the companion PR.