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: add custom enum support in Engine API & TypeScript SDK #7498

Merged
merged 49 commits into from
Jun 21, 2024

Conversation

TomChv
Copy link
Member

@TomChv TomChv commented May 29, 2024

This PR implements custom enum support from a user module and the TypeScript support for it.

Current state

  • Enum TypeDef GraphQL schema extension
  • Enum registration using the GQL API
  • Internal EnumTypeDef support
  • Add ENUM_KIND to TypeDef
  • Complete TypeScript SDK registration system
  • Handle enum field query
  • Handle enum return type
  • Handle enum argument

💡 There's maybe something wrong in my implementation but it works!

Changes

To make that possible, I needed to add a DynamicEnumValue to register dynamic enum in the GraphQL server since the current implementation is static (because of usage of generic)

Also extends the GraphQL schema based on @helderco comments

type EnumTypeDef {
  name: String!
  description: String!
  values: [EnumValueTypeDef!]!
}

type EnumValueTypeDef {
  name: String!
  description: String!
}

type TypeDef {
  """
  If kind is ENUM_KIND, the enum-specific type definition.
  If kind is not ENUM_KIND, this will be null.
  """
  asEnum: EnumTypeDef
    
  """
  Returns a TypeDef of kind Enum with the provided name.
  
  Note that an enum's values may be omitted if the intent is only to refer to an enum.
  This is how functions are able to return their own, or any other circular reference.
  """
  withEnum(name: String!, description: String = ""): TypeDef!

  """
  Adds a static value for an Enum TypeDef, failing if the type is not an enum.
  """
  withValue(name: String!, description: String = ""): TypeDef!
}

enum TypeDefKind {
  """
  A GraphQL enum type and its values.
  
  Always paired with an EnumTypeDef.
  """
  ENUM_KIND
}

Note that only string value is supported for now

Example for TypeScript

import { func, object, field, daggerEnum } from "@dagger.io/dagger"

/**
 * Enum for Status
 */
@daggerEnum()
class Status {
  /**
   * Active status
   */
  static readonly ACTIVE: string = "ACTIVE"

  /**
   * Inactive status
   */
  static readonly INACTIVE: string = "INACTIVE"
}

@object()
export class Enums {
  @field()
  status: Status = Status.ACTIVE

  @func()
  setStatus(status: Status): Enums {
    this.status = status

    return this
  }

  @func()
  getStatus(): Status {
    return this.status
  }
}

💡 Due to TypeScript flexibility, it's pretty tricky to implement it in another way, so I tried to create a syntax close to what we could do in Golang.
The fact that we cannot declare decorators on top of enum is also a limitation.
I'm thinking about a workaround but I think this type of implementation is the cleanest we could get in Typescript.

Examples of commands:

➜  enums git:(main) ✗ dagger call status
ACTIVE

➜  enums git:(main) ✗ dagger call set-status --status FOO status
✔ enums: Enums! 2.4s
✘ Enums.setStatus(status: "FOO"): Enums! 0.0s
! failed to convert arg "status": ModuleEnumType.ConvertToSDKInput: invalid enum value "FOO" for "Status"

Error: response from query: input: enums.setStatus resolve: failed to convert arg "status": ModuleEnumType.ConvertToSDKInput: invalid enum value "FOO" for "Status"

➜  enums git:(main) ✗ dagger call set-status --status INACTIVE get-status
INACTIVE

I think the input part should be handle in the CLI, with possible values displayed, so we can catch the error sooner (/cc @helderco)

Additional changes

This PR also include some improvement on the TypeScript SDK.

  • Deletion of method typedef in abstract module definition in favor of directly calling the method.
    This avoid duplicated function call and simplify the overall code, there's no more 2 concepts of typedef inside the TS
    internals.
  • Load codes representation inside the constructor so results is in memory and not recomputed on every call. This will improve the performances as well as simplify the code.
  • Move typeToTypedef into abstraction directory since it was the only place it was used.
  • Move toPascalCase into module since it was only used there.
  • Delete utils.ts (I've never been a fan of this file tbh).
  • Avoid decorators naming duplication by reusing the decorator name inside registry, this avoid typos.
  • Refactor module.modTypeFor to be more readable and flexible.

Fixes #6780
Fixes OSS-141

@TomChv TomChv self-assigned this May 29, 2024
@TomChv TomChv requested a review from vito May 29, 2024 17:47
@TomChv

This comment was marked as outdated.

@TomChv TomChv changed the title feat: add enum registration methods in Engine API feat: add custom enum support in Engine API & TypeScript SDK May 31, 2024
@TomChv TomChv requested review from helderco, jedevc and sipsma June 4, 2024 00:00
Comment on lines 172 to 176
if (dag[`load${enumType}FromID`]) {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
return dag[`load${enumType}FromID`](value)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Since Enum are not loadable, I should remove this part, and potentielly just return the value, since it should have been validated by the engine first!

@TomChv TomChv force-pushed the feat/enum-support branch 2 times, most recently from 9ee6329 to e697091 Compare June 4, 2024 00:06
@TomChv
Copy link
Member Author

TomChv commented Jun 4, 2024

I need to write some tests but the PRs is ready for a review

@TomChv TomChv marked this pull request as ready for review June 4, 2024 13:08
@TomChv TomChv requested review from a team as code owners June 4, 2024 13:08
core/schema/coremod.go Outdated Show resolved Hide resolved
core/schema/module.go Show resolved Hide resolved
core/schema/module.go Outdated Show resolved Hide resolved
core/typedef.go Outdated Show resolved Hide resolved
Comment on lines 3119 to 3246
/**
* Enum for Status
*/
@daggerEnum()
class Status {
Copy link
Member

Choose a reason for hiding this comment

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

Question: why this syntax instead of typescript's builtin enum type? https://www.typescriptlang.org/docs/handbook/enums.html#string-enums

I think we should stick to the language semantics if possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

💡 Due to TypeScript flexibility, it's pretty tricky to implement it in another way, so I tried to create a syntax close to what we could do in Golang.
The fact that we cannot declare decorators on top of enum is also a limitation.
I'm thinking about a workaround but I think this type of implementation is the cleanest we could get in Typescript.

As explained in my PR's summary.

I cannot implement

@daggerEnum()
enum Status {
    ACTIVE = "ACTIVE"
    // ...
}

Because it's not possible to add a decorator to a TS type or enum, so how can a user explicitely declare the enum must be exposed or not?

Copy link
Member

Choose a reason for hiding this comment

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

Do we need a decorator here? Can we not take the same approach as in go, where we create typedefs for every referenced type - e.g. so if a function accepts/returns an enum, then we automatically codegen for it.

Maybe that's less inline with the rest of the typescript SDK, and makes for more complications.

Copy link
Member Author

@TomChv TomChv Jun 5, 2024

Choose a reason for hiding this comment

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

I mean I wish it worked like that, but I need to refactor a lot of the Typescript internal introspection logic to do so.

Should I work on that? However I would still need decorators to define aliases for example (but I guess we don't want that on enumerations)

Also with Helder we aggreed that explicit declaration is better than implicit, I could add a special case for enum but I don't know, I'm not sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think not being able to use a decorator in an enum shouldn't be an argument against using enum if that's the most natural thing. That said, I remember a user saying they didn't like TypeScript's enums, but I don't know why.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, part of me thinks @function is the only one we could need. Both for class methods and properties. In Python however, @object does change the class, unlike TypeScript. If not that, users would need @dataclass anyway.

Copy link

Choose a reason for hiding this comment

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

I think we should stick to the language semantics if possible.

I strongly agree with this. That being said, enums in typescript can act fairly weirdly. The general way, that at least I use, is to just use a type literal as its more natural in Typescript. But that ends up with the same issue of not being able to add a decorator.

That said, I remember a user saying they didn't like TypeScript's enums, but I don't know why.

I left it on a comment in a PR, I'll try to find it.

Also with Helder we aggreed that explicit declaration is better than implicit, I could add a special case for enum but I don't know, I'm not sure.

The explicit declaration is a little cumbersome in Typescript, as it generally adds a bunch of boiler plate, especially for things like this enum

Copy link

Choose a reason for hiding this comment

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

Here was my original comment on enums.
#7401 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

As I said, not being able to use a decorator shouldn't be an argument against using something else that is more natural. The problem with a type of string literals really is how do you add descriptions to the values?

@TomChv TomChv force-pushed the feat/enum-support branch 2 times, most recently from ec3f36c to 265adea Compare June 10, 2024 11:44
@TomChv
Copy link
Member Author

TomChv commented Jun 10, 2024

dagger call --source=. test custom --run="TestCustomModuleEnumType" passes locally!

We're closer than ever to get this merge!
/cc @jedevc @helderco if you have some time to do the Go & Python integration, that would be amazing 🚀

@TomChv
Copy link
Member Author

TomChv commented Jun 10, 2024

My test passes, the other fails du to flakiness or is it something else?

Screenshot 2024-06-10 at 22 28 18

The fail come from there, it might be from my implementation?

Screenshot 2024-06-10 at 22 29 18

@TomChv
Copy link
Member Author

TomChv commented Jun 12, 2024

Quick update! I finally found a way to fix the issue with interfaces but it's pretty hacky so I'm trying to find a better way.

It's basically an issue in module.go/ModTypeFor function, that doesn't correctly handle the interfaces kind for some reason.
This bug has been introduced after my refactor of this function.

A quick fix is to rollback the change for KindInterface but it's quite ugly...
I want to make it work, I'm digging into a better fix

@TomChv TomChv force-pushed the feat/enum-support branch 3 times, most recently from a38251a to 1d68768 Compare June 13, 2024 18:22
@TomChv
Copy link
Member Author

TomChv commented Jun 13, 2024

Enum tests is passing! @helderco @jedevc You can work on the other languages integrations

Copy link
Member

@jedevc jedevc left a comment

Choose a reason for hiding this comment

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

Left some comments on the engine implementation, will also start looking at how to do enums in go.

dagql/types.go Outdated Show resolved Hide resolved
docs/docs-graphql/schema.graphqls Show resolved Hide resolved
dagql/types.go Outdated Show resolved Hide resolved
dagql/types.go Outdated Show resolved Hide resolved
Tom Chauveau and others added 27 commits June 21, 2024 15:25
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Two parts here:
- Core enums are registered as scalars, so look them up as scalars
- Enums can be converted to each other :) For example, when passing a
  dynamic module enum into a static core enum.

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: Helder Correia <174525+helderco@users.noreply.github.com>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Base class isn't required, just a convenience. Users can make their own enums, but descriptions will only be used if there's a "description" attribute in members.

Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
@helderco helderco merged commit 66e98c3 into dagger:main Jun 21, 2024
44 checks passed
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.

✨ functions: support enum argument types
6 participants