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

feat: improved navigation for codegen #8498

Merged
merged 8 commits into from
Oct 29, 2024
Merged

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Sep 19, 2024

This is kind of a bundle of various different patches all to enable "better multi-module navigation". With the incoming work in #8355, we're gonna have even more modules than we currently do today (and we already have a few). Working on a multi-module project is quite difficult, and it's challenging to navigate between functions defined in different modules.

The end goal is to produce something like this (for a module dep that contains a ContainerEcho function), note the dep/src/main.go:31 at the end of the function declaration:

// Returns a container that echoes whatever string argument is provided
func (r *Dep) ContainerEcho(stringArg string, a string, opts ...DepContainerEchoOpts) *Container { // dep (../../dep/src/main.go:31)
	q := r.query.Select("containerEcho")
	for i := len(opts) - 1; i >= 0; i-- {

Most IDEs include support clicking on this link, and navigating to the right place automatically - a future IDE plugin could automatically find this and jump to it.

To do this:

  • The SDK typedef API is updated to allow attaching source-maps. These are a record of the filename, the line number, and the column number that a typedef declaration corresponds to in the source. Each SDK analyzes the source code, and outputs these when creating the typedefs during module initialization.
  • The engine maps these source-maps from the typedefs onto graphql directives.
  • The graphql introspection API is extended to allow querying the directives on a schema
  • Each SDK codegen outputs the source-maps as line comments of the form ./path/to/filename:line next to the generated code.

TODO:

  • Basic implementation
  • Add support for the remaining typedefs: enums, scalars, interfaces, etc.
  • Final bits and pieces
    • Parent paths don't work?
      • That path should always be relative from the file it's written to
    • Test drive our CI
  • Cleanup 🧹
  • Tests 🔨
  • More SDK support (maybe a future PR)

@jedevc jedevc force-pushed the better-gen-nav branch 3 times, most recently from d1b2ddf to 17a297b Compare September 23, 2024 16:33
@jedevc
Copy link
Member Author

jedevc commented Sep 23, 2024

This PR should now be ready to review, it's not 100% feature complete, since it's missing other language support, but IMO it's a good enough starting point to get in.

@jedevc jedevc marked this pull request as ready for review September 23, 2024 16:34
@jedevc jedevc requested review from a team as code owners September 24, 2024 13:57
@jedevc jedevc requested a review from a team as a code owner September 24, 2024 14:34
@jedevc
Copy link
Member Author

jedevc commented Sep 24, 2024

Woo, think I've really got the edge cases now. I've also split it out into smaller commits to make it easier to review.

Comment on lines +841 to +850
func (d *DirectiveApplication) Type() *ast.Type {
return &ast.Type{
// Some clients don't like custom introspection types like this :(
// NamedType: "__DirectiveApplication",
NamedType: "_DirectiveApplication",
NonNull: true,
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to the reviewer: this is very sad 😢

Ideally, I'd love to be able to define our own "internal" types, and it looked for a while like I could, but python's graphql-core really did not like this: https://github.com/graphql-python/graphql-core/blob/4e83d4201a2be88832f24c5733c0a1b8568c3708/src/graphql/type/validate.py#L181-L196

So, we can just use a single underscore here, and just modify the codegen for the clients to handle this case.

Comment on lines +37 to +39
if strings.HasPrefix(schemaType.Name, "_") {
continue
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to the reviewer: this change is required because previously we were relying on graphql's typescript library to filter out __ prefixes when creating a schema from an introspection result.

But, with the change to use _ prefixes for "dagger internal" in some places, we need an extra processing step here that we didn't need before.

Copy link
Contributor

@sipsma sipsma left a comment

Choose a reason for hiding this comment

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

🎉

@@ -34,6 +34,8 @@ type Config struct {
ModuleName string
// ModuleContextPath is the subpath where a module can be found.
ModuleContextPath string
// ModuleParentPath is the path from the subpath to the output
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can this comment be clarified more? The one you have later of // path from the module root to the context was helpful and parsable by my brain 😄

I got this working for the type values previously in
87aed89, but woops - we also need it
for the type itself.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Previously, only the python SDK was filtering out any types beginning
with a single underscore "_" - all the other SDKs + APIs were assuming
that only the double underscore should be filtered out "__".

However, because we want to add our own internal types (and we can't add
double underscore prefixes, grr), then we should also filter out any
underscored type (which seems absolutely fine to do).

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc merged commit ca92eab into dagger:main Oct 29, 2024
57 checks passed
@jedevc jedevc deleted the better-gen-nav branch October 29, 2024 14:36
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.

2 participants