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

✨ TypeScript SDK: resolve introspection by reference #8676

Closed
8 tasks
TomChv opened this issue Oct 10, 2024 · 12 comments · Fixed by #8824
Closed
8 tasks

✨ TypeScript SDK: resolve introspection by reference #8676

TomChv opened this issue Oct 10, 2024 · 12 comments · Fixed by #8824
Assignees

Comments

@TomChv
Copy link
Member

TomChv commented Oct 10, 2024

The current TypeScript introspector is quite limited and remove a bunch of interesting feature that are natively available and broadly uses by TS engineers such as the complete type system, generics and complex default value.

The global idea is to make the TS SDK DX much more natural to a TS engineer.

For example, this simple snippet isn't working with dagger but should after this improvement:

type User = {
  name: string
  age: number
}

const DEFAULT_NAME = "john"

@object()
export class Test {

  // This is broken because `User` will not be registered in the Dagger API.
  @func() 
  greet(user: User): string {
     return `Hey I'm ${user.name}, ${user.age.toString()} years old`
  }

  // This is broken because `DEFAULT_NAME` isn't resolved during the introspection.
  @func()
  newPerson(name = DEFAULT_NAME): User {
     return { name, age: 19 }
  }
}

Right now, you need to create a class User, exposing field using decorator and creating a constructor if you don't want to assign fields one by one. It's possible to hack it using Object.assign but it's doesn't feels right and it's not commonly used in TypeScript project.

Then why don't we just supports the typekeyword?

The answer is simple: how do we know if the type should actually be exposed to the API or not? We managed that with decorators like @object() and @func() but type, enum and interface keywords doesn't support decorators.
So if we aim to support these keywords, we would need to resolve types & value when they are references.

How does this work

If we reuse the same example, the flow of introspection would be:

  1. Get the module name, convert it to PascalCase with will result in Test
  2. Search for the Test class in the user's module code (verify that it's decorated)
  3. Register existing functions and detect if there's a reference to an external declaration
  4. Lookup all reference (here it would be User and DEFAULT_NAME) and register them (for default value it's just resolving what the value is)
  5. Continue recursively until there's no more reference.

What does it enable

This would also enable constructing complex nested type without a huge boilerplate such as

type File = {
   name: string
   size: number
}

type Directory = {
  files: File[]
  path: string
}

Instead of

@object()
class File {
  @func()
  name: string

  @func()
  size: number

  constructor(
    name: string
    size: number
  ) {
     this.name = name
     this.size = size
  }
}

// ... I think it's pretty explicit for the rest

And also indeed, resolve default value in a more accurate way.

It would also be an occasion to improve the introspection error handling to give more hint to the user.
For example: type XXX is referenced in function XXX but isn't exposed to the dagger API. Please decorate your class with @object(), same for specific types that are not supported yet like type Status = "online" | "offline".

Specs of the new supported TS features

⚠️ ALL these new keywords would be supported implicitely since they cannot be decorated.

type

Can be used to create dagger object but we can imagine supporting union types also (in a simple way for now)

// A dagger object with a field name & age
// For now properties are public (but there might be ways to actually make it private with generics)
type User {
   name: string
   age: number
}

enum

Resolving references would also enabling using native TypeScript eum, instead of our current noisy notation.
It's not perfect though since you cannot add metadata yet, but this can be solved with generic (gonna write an issue soon about that)

// This could be a better notation for enum also (can be supported in a follow up)
enum Status {
  Online = "online"
  Offline = "Offline"
}

💡 interface keyword will not be supported yet since it's pretty confusing with actual Dagger interface. TS interface allows to create data structure like type but also containing function, it's more or less a class with only public properties and no constructor. Making it very hard to pick in the context of Dagger.

Referenced default value

Thanks to #8653, the introspection will be done in 2 phases.

  1. Check what's exported from the user's module (any function, class etc... can be resolved, meaning that an exported default value can be resolved and registered to the GraphQL!)
  2. Resolve class, values and use the introspector for specific data.

Tasks

@Xiot
Copy link

Xiot commented Oct 11, 2024

I am very excited for this. Wanted to point out some incorrect assumptions with this though.

💡 interface keyword will not be supported yet since it's pretty confusing with actual Dagger interface. TS interface allows to create data structure like type but also containing function, it's more or less a class with only public properties and no constructor.

The differences between type and interface are actually a lot more subtle that you described. Both of them can have fields and methods, and both can be composed (extends / implements)

The interface in Typescript is pretty much like an interface in 'normal' OOP languages. It simply defines the shape of an object, but doesn't supply any of the implementation.

interfaces can also be extended, so they can have "partial" definitions in separate files.

type can be as simple as you have in your original post, or they can get really complex. The type system is actually turing complete.

A very simple usage of the type system would be

type PartialUser = Partial<User>

Partial is built into the language, but its definition looks something like

type Partial<T extends object> = {
  [K in keyof T]?: T[K]
}

That iterates through all of the properties of T and makes them optional. So,

type PartialUser = Partial<User>

is the same as

type PartialUser = {
  name?: string
  age?: number

I'm not suggesting having full support for these types, but just wanted to expand your knowledge on them a little bit more

@TomChv
Copy link
Member Author

TomChv commented Oct 14, 2024

The interface in Typescript is pretty much like an interface in 'normal' OOP languages. It simply defines the shape of an object, but doesn't supply any of the implementation.

I totally agree! However in the context of dagger, an Interface is the same as a Go interface, which is quite different that OOP since you cannot have any property in an Go interface. Originally, I wanted to support both type and interface but after talking with some of my peers and seeing how much they got lost between interface & dagger interface, I rollbacked my idea.

type can be as simple as you have in your original post, or they can get really complex. The type system is actually turing complete.

Of course! I tried to kept it scope of what we want to support. It would be too much of work for now to support specific Partial, Require, Pick, Omit etc... but maybe one day ;)

@Xiot
Copy link

Xiot commented Oct 14, 2024

Originally, I wanted to support both type and interface but after talking with some of my peers and seeing how much they got lost between interface & dagger interface

This is where some of my concerns come in on the ts-sdk side, since most thinks, it seems (from my perspective, anyway) is based on go, and not on typescript. It's the same as the @field() vs @func().

I would assume that the majority of people using the typescript sdk, would have a lot more experience with typescript than go, so having some go'isms creep into the ts-sdk feels a little out of place.

@TomChv
Copy link
Member Author

TomChv commented Oct 15, 2024

I would assume that the majority of people using the typescript sdk, would have a lot more experience with typescript than go, so having some go'isms creep into the ts-sdk feels a little out of place.

So you think I should support both interface and type but then how would you define a dagger interface?

If interface can be use to describe a dagger object, what's left to define a dagger interface?

@Xiot
Copy link

Xiot commented Oct 15, 2024

If interface can be use to describe a dagger object, what's left to define a dagger interface?

I don't really know enough about a "dagger interface" to be of much help for that question, but I would think that interface and dag.Interface would be different, unrelated things.

So you think I should support both interface and type

I'm just saying that some people would instinctively use interface when definining their local types. You could just support one, but it does still get a little messy. Especially if interface is overloaded to be restricted to dag.Interface

@TomChv
Copy link
Member Author

TomChv commented Oct 17, 2024

UPDATE

I'm dropping some tests I did to check if importing module could help on the introspection and it seems were very limited with the current configuration:

This is the example I've used:

Code
// index.ts
import { func, object } from "../../../../introspector/decorators/decorators.js"
import { Data, defaultEnum, TestEnum } from "./types.js"

/**
 * References class
 */
@object()
export class References {
  @func()
  data: Data[]

  constructor(data: Data[] = []) {
    this.data = data
  }

  @func()
  addData(data: Data): References {
    this.data.push(data)

    return this
  }

  @func()
  dumpDatas(): Data[] {
    return this.data
  }

  @func()
  testEnum(test: TestEnum = defaultEnum): TestEnum {
    return test
  }

  @func()
  testEnumStatic(test: TestEnum = TestEnum.A): TestEnum {
    return test
  }

  @func()
  testDefaultValue(foo: string = "a"): string {
    return foo
  }
}
// type.ts

/**
 * Data
 */
export type Data = {
  /**
   * Item 1
   */
  item1: string
  item2: number
}

/**
 * Test Enum
 */
export enum TestEnum {
  /**
   * Field A
   */
  A = "a",

  /**
   * Field B
   */
  B = "b",
}

export const defaultEnum = TestEnum.A
  1. Cool thing: I can get any exported class/definition/value from a typescript file:
[
  [Module: null prototype] {
    TestEnum: { A: 'a', B: 'b' },
    defaultEnum: 'a'
  },
  [Module: null prototype] { References: [class References] }
]

However, this doesn't include types and interfaces.

  1. Bad thing: I cannot actually introspect these values, it's hidden by the JS engine and I hit the following error:
TypeError: 'caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them

Based on some research, it's a normal behaviour and we shouldn't disable strict mode to access these data.

Accessing caller, callee, and arguments was restricted in strict mode to improve security and allow JavaScript engines to optimize function calls better. These properties are mostly deprecated due to their performance impacts and the potential for errors in asynchronous contexts.

In conclusion

I can use the import logic to actually resolve exported value, which can be useful for default value resolution but I'm pretty much stuck on the introspection side. I'll need to do it with the basic TypeScript compiler API.

I'll continue following this path, if by any chance there's a better way, please let me know, I'm writing code that should be easily replacable for that purpose.

@Xiot
Copy link

Xiot commented Oct 17, 2024

If you are just using javascript, then you definitely won't access to the types.
For this stuff it will all need to be with the compiler api.

There is also a way to get the compiler api to simplify a type for you. So it would take something like

type User = {name: string, age: number}
type PartialUser = Partial<User>

and give you

type PartialUser = {name?: string, age?: number}

I can't remember the exact way off of the top of my head, but it did involve getting the symbols for the types IIRC

@TomChv
Copy link
Member Author

TomChv commented Oct 17, 2024

I can't remember the exact way off of the top of my head, but it did involve getting the symbols for the types IIRC

This will definitely be helpful! I almost at this part haha

@Xiot
Copy link

Xiot commented Oct 17, 2024

That would also simplify the 'do we use type or interface for dag.Interface' question.
As long as you can simplify the type, it doesn't matter which one is used.

There is also ts-morph which may help out in using the compiler api

see: https://stackoverflow.com/a/68623960

This may help as well. The fact that its a cli doesn't help much, but it does basically achieve what you need
https://github.com/FormidableLabs/ts-simplify

@TomChv
Copy link
Member Author

TomChv commented Oct 17, 2024

I'm currently hitting an issue with ts-morph, I'll need to change the way I actually resolve typedef in case of intersection.

I opened an issue, hopefully I can get unblock fast on that because it's another level of difficulity to do an hybrid resolution: dsherret/ts-morph#1580

@TomChv
Copy link
Member Author

TomChv commented Oct 21, 2024

Update related to the Type resolution:

I'm having an issue with ts-morph, he's not able to properly resolve some types for some reason so I'll need to change my strategie and fallback to backward resolution.

Basically the idea is:

  1. Solve what I can (and easy type)
  2. If I cannot solve it (if the type is a reference to another type): store it as undefined until it's solved by reference later.

Because sometime, ts-morph will think a type is something while it's actually another, example:

Here's in the first case, type is correctly resolve but not in the other because the default value seems to impact the type resolution (ts-morph tries to do some inference but it doesn't really work well).

  @func()
  testEnum(test: TestEnum = defaultEnum): TestEnum {
    return test
  }

  @func()
  testEnumStatic(test: TestEnum = TestEnum.A): TestEnum {
    return test
  }
        "testEnum": {
          "name": "testEnum",
          "arguments": {
            "test": {
              "type": {
                "kind": "ENUM_KIND",
                "name": "TestEnum"
              },
              "defaultValue": "b"
            }
          }
        },
        "testEnumStatic": {
          "name": "testEnumStatic",
          "arguments": {
            "test": {
              "type": {
                "kind": "OBJECT_KIND",
                "name": "References"
              },
            }
          }
        },

@TomChv
Copy link
Member Author

TomChv commented Oct 24, 2024

Ts-morph is doing such a weird job on type resolution.. it can resolve a node to a complete different type and it's not consistent...

node: test: TestEnum = TestEnum.A 
type: import("/Users/tomchauveau/Documents/github.com/dagger/dagger/sdk/typescript/introspector-v2/test/testdata/references/types").Data[]

That's so strange...

I've complete the reference propagation, it works perfectly! Now I'm fighting with ts-morph not giving me the correct type for an argument.

Same for return type:

returnType displayed: References 
return type found by ts-morph: Data

And event for properties

{
  rawStringProperty: ")\n  foo: number = 4",  
  rawTypeString: "number", // The typedef I have from a manual parsing
  typeDef: {
    kind: "STRING_KIND", // The typedef I get from ts-type
  },
  node: "@func(\"oof\")\n  foo: number = 4", // The node ts-morph stores it has
  typeFromNode: "string", // What ts-morph tells me
}

TomChv pushed a commit to TomChv/dagger that referenced this issue Oct 30, 2024
Fixes dagger#8676.

Refactor typedef resolution to be resolve by reference.
The introspection starts from the entrypoint (which is the class named
the same as the module) and recursively resolve every references.

Refactor the TS compiler API to be abstract with a AST that handle
lookups, typedef resolutions and default value resolution.

Use `imports` to resolve default value that refers to an exported variable.

Add custom introspection error.

Improve error handling to point to the line that couldn't be introspected.

Add supports for native `enum` keyword and `type` native keyword.

Simplify overall code abstractions too minimal.
Avoid long abstractions functions.

Signed-off-by: Tom Chauveau <[email protected]>
TomChv pushed a commit to TomChv/dagger that referenced this issue Oct 30, 2024
Fixes dagger#8676.

Refactor typedef resolution to be resolve by reference.
The introspection starts from the entrypoint (which is the class named
the same as the module) and recursively resolve every references.

Refactor the TS compiler API to be abstract with a AST that handle
lookups, typedef resolutions and default value resolution.

Use `imports` to resolve default value that refers to an exported variable.

Add custom introspection error.

Improve error handling to point to the line that couldn't be introspected.

Add supports for native `enum` keyword and `type` native keyword.

Simplify overall code abstractions too minimal.
Avoid long abstractions functions.

Signed-off-by: Tom Chauveau <[email protected]>
TomChv pushed a commit to TomChv/dagger that referenced this issue Nov 7, 2024
Fixes dagger#8676.

Refactor typedef resolution to be resolve by reference.
The introspection starts from the entrypoint (which is the class named
the same as the module) and recursively resolve every references.

Refactor the TS compiler API to be abstract with a AST that handle
lookups, typedef resolutions and default value resolution.

Use `imports` to resolve default value that refers to an exported variable.

Add custom introspection error.

Improve error handling to point to the line that couldn't be introspected.

Add supports for native `enum` keyword and `type` native keyword.

Simplify overall code abstractions too minimal.
Avoid long abstractions functions.

Signed-off-by: Tom Chauveau <[email protected]>
TomChv pushed a commit to TomChv/dagger that referenced this issue Nov 13, 2024
Fixes dagger#8676.

Refactor typedef resolution to be resolve by reference.
The introspection starts from the entrypoint (which is the class named
the same as the module) and recursively resolve every references.

Refactor the TS compiler API to be abstract with a AST that handle
lookups, typedef resolutions and default value resolution.

Use `imports` to resolve default value that refers to an exported variable.

Add custom introspection error.

Improve error handling to point to the line that couldn't be introspected.

Add supports for native `enum` keyword and `type` native keyword.

Simplify overall code abstractions too minimal.
Avoid long abstractions functions.

Signed-off-by: Tom Chauveau <[email protected]>
TomChv pushed a commit to TomChv/dagger that referenced this issue Nov 13, 2024
Fixes dagger#8676.

Refactor typedef resolution to be resolve by reference.
The introspection starts from the entrypoint (which is the class named
the same as the module) and recursively resolve every references.

Refactor the TS compiler API to be abstract with a AST that handle
lookups, typedef resolutions and default value resolution.

Use `imports` to resolve default value that refers to an exported variable.

Add custom introspection error.

Improve error handling to point to the line that couldn't be introspected.

Add supports for native `enum` keyword and `type` native keyword.

Simplify overall code abstractions too minimal.
Avoid long abstractions functions.

Signed-off-by: Tom Chauveau <[email protected]>
TomChv pushed a commit to TomChv/dagger that referenced this issue Nov 14, 2024
Fixes dagger#8676.

Refactor typedef resolution to be resolve by reference.
The introspection starts from the entrypoint (which is the class named
the same as the module) and recursively resolve every references.

Refactor the TS compiler API to be abstract with a AST that handle
lookups, typedef resolutions and default value resolution.

Use `imports` to resolve default value that refers to an exported variable.

Add custom introspection error.

Improve error handling to point to the line that couldn't be introspected.

Add supports for native `enum` keyword and `type` native keyword.

Simplify overall code abstractions too minimal.
Avoid long abstractions functions.

Signed-off-by: Tom Chauveau <[email protected]>
TomChv pushed a commit to TomChv/dagger that referenced this issue Nov 19, 2024
Fixes dagger#8676.

Refactor typedef resolution to be resolve by reference.
The introspection starts from the entrypoint (which is the class named
the same as the module) and recursively resolve every references.

Refactor the TS compiler API to be abstract with a AST that handle
lookups, typedef resolutions and default value resolution.

Use `imports` to resolve default value that refers to an exported variable.

Add custom introspection error.

Improve error handling to point to the line that couldn't be introspected.

Add supports for native `enum` keyword and `type` native keyword.

Simplify overall code abstractions too minimal.
Avoid long abstractions functions.

Signed-off-by: Tom Chauveau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants