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] Support string literal types #7401

Open
Xiot opened this issue May 17, 2024 · 16 comments
Open

✨ [Typescript] Support string literal types #7401

Xiot opened this issue May 17, 2024 · 16 comments

Comments

@Xiot
Copy link

Xiot commented May 17, 2024

What are you trying to do?

I'm trying to use a literal type in order to have better typing in my dagger code.

I have a field like this on an object.

@field() type: 'head' | 'merge'

However it looks like the code-gen doesn't seem to support this as when I try to run the dagger module, I get

 Error: query module objects: returned error 400 Bad Request: failed to get schema introspection JSON: introspection query failed: input: __schema.types[75].fields[2].type.ofType panic while resolving __Type.ofType: unknown type: headmerge

Why is this important to you?

Typing is important

How are you currently working around this?

Currently, I am just using a string for the field and am casting it later.

@Xiot
Copy link
Author

Xiot commented May 17, 2024

Related to this, I just got this since my module was returning a fixed set of static string.
You most likely just need to change the representation to a string if you detect a string literal while traversing the AST

query module objects: returned error 400 Bad Request: failed to get schema for module "motion-dagger": failed to create function "on": failed to find mod type for function "on" return type: "TaskNotNeededOnDraftTaskNotNeededOnChoreNoChangeNeededOk!"

@TomChv TomChv added the sdk/typescript All about TypeScript sdk label May 22, 2024
@TomChv
Copy link
Member

TomChv commented May 22, 2024

@Xiot Thanks for reporting this issue, we could solve this by allow user to implement their own Scalar or enum.

We made a PR few weeks ago to add support for Scalar, however it's only internal values for now.

Here's also a related issue with enumeration #6780

@helderco Should I start to work on an implementation to support custom scalar registration?

Related to this, I just got this since my module was returning a fixed set of static string.
You most likely just need to change the representation to a string if you detect a string literal while traversing the AST

Yes! And the idea is to have a better readability by supporting Scalar or Enum, I'm not sure we want to support inline union type since it's specific to TypeScript and not Go nor Python can do that kind of things.

So you would have

// OR Enum
type TaskStatus = "TaskNotNeededOnDraft" | "TaskNotNeededOnChore" | "NoChangeNeeded" | "Ok!"

@func()
fct(): TaskStatus {}

@Xiot
Copy link
Author

Xiot commented May 22, 2024

This is great
From a Typescript perspective I prefer unions of string literal over enums. Its generally closer to what I tend to use, as enums in typescript are weird

@TomChv
Copy link
Member

TomChv commented May 22, 2024

From a Typescript perspective I prefer unions of string literal over enums. Its generally closer to what I tend to use, as enums in typescript are weird

I totally agree, and right now we're closer than even to support custom unions (called Scalar in our case)

@helderco
Copy link
Contributor

@helderco Should I start to work on an implementation to support custom scalar registration?

Please don't. What OP is asking is clearly an enumeration, not a custom scalar:

That is useful 👍 and I also want it. 🙂

Yes! And the idea is to have a better readability by supporting Scalar or Enum, I'm not sure we want to support inline union type since it's specific to TypeScript and not Go nor Python can do that kind of things.

Ahem... Python does support unions (and much more complex stuff). 🙂 It's just Go that doesn't. But what you need to consider the most is GraphQL. GraphQL does have unions but we haven't discussed using them in Dagger. They share similarities with interfaces (interfaces dictate which fields must be common, while unions do not), so maybe.

Note that GraphQL unions represent a selection of allowed concrete types. Not values like OP is asking. It's enumerations that restrict to a particular set of values.

You could have the SDK translate a union of literals to an enum automatically, but you also need a name for the enum, and why have two ways of doing the same thing?

@helderco
Copy link
Contributor

This is great From a Typescript perspective I prefer unions of string literal over enums. Its generally closer to what I tend to use, as enums in typescript are weird

What makes enums weird? I'm not familiar.

Note that enums need a name, a set of string values, a description for the enum and a description for each value. A union of literals doesn't give you the descriptions. I'm not sure about the name.

@Xiot
Copy link
Author

Xiot commented May 23, 2024

Javascript itself doesn't support enums, the closest thing that it has is an object.
Typescript adds some semantic sugar to support them, but it still gets turned into an object when it compiles down to JS.

Here is a typescript playground that shows enum and const enum
https://tsplay.dev/m3dLjW

The following typescript

enum Foo {
  first = 1,
  second = 2,
}

const enum Bar {
  first = "a",
  bar = 4,
}

console.log("values", Foo.first, Bar.bar);

Get turned into this.

"use strict";
var Foo;
(function (Foo) {
    Foo[Foo["first"] = 1] = "first";
    Foo[Foo["second"] = 2] = "second";
})(Foo || (Foo = {}));
console.log("values", Foo.first, 4 /* Bar.bar */);

The resulting Foo can be indexed by either the name or the value

Foo.first === 1
Foo[1] === 'first'

For this specific case string literal types, it doesn't even need to make it to the graphql layer and could just be a string under the hood. Although this does depend on how your representation of the data.

Note that GraphQL unions represent a selection of allowed concrete types. Not values like OP is asking. It's enumerations that restrict to a particular set of values.

The way that its modelled in typescript (at least from the AST / typechecker perspective) is that the string literals are themselves a concrete type, so in that sense its more like a scalar.

@Xiot
Copy link
Author

Xiot commented May 23, 2024

Note that enums need a name, a set of string values, a description for the enum and a description for each value. A union of literals doesn't give you the descriptions. I'm not sure about the name.

What makes a union of string literals nice is that it doesn't need anything besides the value. Most of the type, they have a type alias referencing them, so that you don't need to constantly re-type all of the values when you need to re-use it. From the callers perspective, they are nice because they "just work" and you don't need to reference another object in order to get proper typechecking / intellisense.

On a bit of a tangent, one of the things that get make the TS SDK slightly hard to use, is that effectively only part of the language is valid. There are a bunch of gotchas (like this and #7446) that a TS dev will use without thinking, but it results in some undefined behaviour or code-gen errors.

@helderco
Copy link
Contributor

Well, the properties thing can probably be changed to be more natural. It's also possible to report the union of literals as just a string but wouldn't let other sdks that use the ts module know about the set of allowed values. The value descriptions for enums aren't required but the sdk does need to allow setting them.

Please understand that what a Dagger Module really does is expand the Dagger API. Modules add more types and fields to the GraphQL schema. And this is the common denominator that allows the ecosystem the flexibility of a TypeScript module being able to reuse and call functions in a Go module, and vice versa.

As long as the TypeScript SDK can correctly report the necessary types for schema expansion in a way that it's a good citizen to the ecosystem, anything goes to make the DX as natural as possible to TypeScript developers.

We appreciate all the help the community can give and the feedback is valuable. Please report anything out of place that you may find.

@helderco
Copy link
Contributor

helderco commented May 23, 2024

One small detail about the descriptions that I forgot to mention is that if things don't have descriptions, it'll be a worse experience when exploring the module via the CLI, Daggerverse or even in another language's autocomplete in the IDE, when used as a dependency. That's what I meant by good citizen.

(edit: the good citizen is the SDK first of all, and then anyone that want to share code with others)

@Xiot
Copy link
Author

Xiot commented May 23, 2024

A description could be constructed from the values of the union, but I understand what you mean. And I do also understand that the majority of your users are in go. I thought about switching my stack to use the go sdk instead so I would get a better feel for the ecosystem.

I also know that I may not be the target audience for the concept of the daggerverse and expanding the dagger api, as my use case is more around creating what I need for my specific use-case rather than pulling in off-the-shelf extensions.

One of the other feature requests that I raised, #7402, was to create a series of lint rules that would make it more clear that something, in this case string literals as the type for a field or func, wasn't supported. Would be an easy way to guide typescript developers to avoid patterns that would cause code-gen errors and point them to more supported alternatives.

That is definitely a lower priority, though

@helderco
Copy link
Contributor

helderco commented May 23, 2024

[...] I do also understand that the majority of your users are in go. I thought about switching my stack to use the go sdk instead so I would get a better feel for the ecosystem.

That's not necessary because with Dagger it doesn't matter which language you choose. It's not specifically about Go, just that there's other languages and they all interact with each other.

The point is you can write a TypeScript module and still reuse code/modules written in another language. And you don't have to know which language those modules are written in.

There's simply more Go modules out there, but that doesn't give you an "edge" in itself if you'd chosen the Go stack. I think it's more important to choose the language that you're more productive in, or that you enjoy the most. 🙂

I also know that I may not be the target audience for the concept of the daggerverse and expanding the dagger api, as my use case is more around creating what I need for my specific use-case rather than pulling in off-the-shelf extensions.

That's fine, I meant that it's the TypeScript SDK, as with all of our SDKs, that need to properly support the ecosystem, through the common denominator that is our GraphQL API.

That's why we can't add support for a literal string union as enumeration feature because it won't allow any TS user to document it properly.

Documentation in a module is helpful not just for sharing to a global audiance. It's also helpful in a small team in a private company so that teammates can understand, or even remember, what options are available, without having to read code.

One of the other feature requests that I raised, #7402, was to create a series of lint rules that would make it more clear that something, in this case string literals as the type for a field or func, wasn't supported. Would be an easy way to guide typescript developers to avoid patterns that would cause code-gen errors and point them to more supported alternatives.

Feature requests and bug reports are definitely appreciated, especially if they make the DX more natural to other users of the same language. 🙂 The only problem is we have limited bandwidth and TypeScript knowledge in our team. I personally don't understand what you're asking there as I'd have to research it. 😇 I defer to @TomChv as the resident TS expert 🙂

So if you know what needs to change, PRs are always welcome. 🙏

@Xiot
Copy link
Author

Xiot commented May 23, 2024

That's not necessary because with Dagger it doesn't matter which language you choose. It's not specifically about Go, just that there's other languages and they all interact with each other.

I agree that I don't have to know what stack a given module was written in, but given that go is more restrictive of the bunch, and that your expertise is greater in go, means that the DX will probably be more 'native' in go.

I think it's more important to choose the language that you're more productive in, or that you enjoy the most.

The main issue that I'm running into with the TS sdk, is that I'm losing productivity due to the way that it interacts with the dagger ecosystem. There are things, like string literals, that make me more productive in TS which aren't compatible with dagger. The way that javascript handles async code, also doesn't work as nicely with dagger as it would with go.

The only problem is we have limited bandwidth and TypeScript knowledge in our team. I personally don't understand what you're asking there as I'd have to research it.

I completely understand. For this issue in particular, its probably safe to close it. I will try to find some time to handle #7402 to warn myself, and others, that somethings aren't possible when creating a dagger module.

@TomChv
Copy link
Member

TomChv commented May 27, 2024

One of the other feature requests that I raised, #7402, was to create a series of lint rules that would make it more clear that something, in this case string literals as the type for a field or func, wasn't supported. Would be an easy way to guide typescript developers to avoid patterns that would cause code-gen errors and point them to more supported alternatives.

Yes that would be nice for users to have a linter that check if the feature is supported or not, another way around would be to verify the code itself and check for unsupported feature so the codegen can throw an error.

The main issue that I'm running into with the TS sdk, is that I'm losing productivity due to the way that it interacts with the dagger ecosystem.

Yes that's an unfortunate tradeoff, TypeScript is much more flexible than Go.

I'm not sure adding lint rules would actually helps, you would still need to adapt the code to work as a module (at least you would know before I agree). The best we could do is integrate more and more features until we can use the full power of the language.
But as I said, we also need to be consistent with every languages. We're working on improving the DX and extending the feature so hopefully, with time, we'll reach that point 🚀

@Xiot
Copy link
Author

Xiot commented May 27, 2024

Yes that would be nice for users to have a linter that check if the feature is supported or not, another way around would be to verify the code itself and check for unsupported feature so the codegen can throw an error.

I wouldn't really call that "another way around". The codegen should probably be the one that is catching these errors. The lint rule is just a layer on top that warns the user while they are still developing

I'm not sure adding lint rules would actually helps, you would still need to adapt the code to work as a module (at least you would know before I agree).

That what the point of a lint rule though. They are generally designed to warn you if you are doing something that is unwanted/unsafe. But there would definitely need to be a separate set of lint rules for functions vs modules

@TomChv
Copy link
Member

TomChv commented May 27, 2024

That what the point of a lint rule though. They are generally designed to warn you if you are doing something that is unwanted/unsafe. But there would definitely need to be a separate set of lint rules for functions vs modules

Yeah that's definitely something we can explore!

@samalba samalba removed the sdk/typescript All about TypeScript sdk label May 31, 2024
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

No branches or pull requests

4 participants