-
Notifications
You must be signed in to change notification settings - Fork 557
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
Comments
Related to this, I just got this since my module was returning a fixed set of static string.
|
@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?
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 {} |
This is great |
I totally agree, and right now we're closer than even to support custom unions (called |
Please don't. What OP is asking is clearly an enumeration, not a custom scalar: That is useful 👍 and I also want it. 🙂
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? |
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. |
Javascript itself doesn't support enums, the closest thing that it has is an object. Here is a typescript playground that shows 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.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.
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. |
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. |
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. |
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) |
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 That is definitely a lower priority, though |
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. 🙂
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.
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. 🙏 |
I agree that I don't have to know what stack a given module was written in, but given that
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.
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. |
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.
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. |
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
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! |
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.
However it looks like the code-gen doesn't seem to support this as when I try to run the dagger module, I get
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.The text was updated successfully, but these errors were encountered: