Conversation
|
@avatus @gzdunek This is what I cooked up. There's 35 remaining type errors in 19 files but I'll leave that for later. Submitting this as a WIP because I don't have more time for this today. In a sound type system, we'd have to validate incoming values to ensure they actually match the type. That is, when making an RPC and getting some objects, we'd have to make a runtime check to verify that the URI indeed matches the type. In TypeScript, we can cheat a little and just say that we trust that anything that comes from tshd is the type it purports to be. I have to say this is pretty nice. I already made a couple of mistakes while fixing the types in tests and TypeScript immediately caught those. I'm unsure if there are any unforeseen roadblocks ahead, but I think if we ensure any incoming objects are properly typed (for now it's just stuff from tshd) and that docs & gateways are created with correct types, I think there's nothing which would prevent us from using this approach. |
| export type RootClusterServerUri = | ||
| `/clusters/${RootClusterId}/servers/${ServerId}`; |
There was a problem hiding this comment.
It would be great if we could restrict RootClusterId to a stricter type then string.
Currently, this is valid code because foo/leaves/bar is a string so it's a valid value for RootClusterId.
const uri1: RootClusterServerUri = '/clusters/foo/leaves/bar/servers/quux'
const uri2: RootClusterUri = '/clusters/foo/leaves/bar/servers/quux'
const uri3: RootClusterUri = '/clusters/foo/leaves/bar'If it's not possible, then perhaps we shouldn't define types which cannot be enforced and instead focus just on those that we actually can. So we'd be able to differentiate a document URI from a resource URI, server URI from database URI. But we wouldn't be able to differentiate a root cluster URI from a leaf cluster URI.
This would still provide some value.
There was a problem hiding this comment.
I mean, technically the current implementation would at least prohibit us from passing a root cluster URI where a leaf cluster URI is expected. But I don't think this is that useful, we don't have many places where leaf cluster URIs are allowed but root cluster URIs are prohibited. And it doesn't outweigh the false positive of passing leaf cluster URI as a root cluster URI.
There was a problem hiding this comment.
It would be great if we could restrict RootClusterId to a stricter type then string.
AFAIK it is not possible, hopefully in the future we will be able to create regex-validated string types.
If it's not possible, then perhaps we shouldn't define types which cannot be enforced and instead focus just on those that we actually can.
Hmm, I would define them anyway. They would act as type aliases. I think it is still better for the reader to see clusterUri: RootClusterUri than clusterUri: string. You could also add a short comment to the type definition with an example.
There was a problem hiding this comment.
I don't think our types need to differentiate between root/leaf (i mean... if it was an easy fix sure), but as you said, there is nowhere that expects leaf only, just places that require root only.
I agree that defining them anyway would help future readers. not only would we have params named stuff like rootClusterUri but also typed for double safety.
There was a problem hiding this comment.
I think it is still better for the reader to see
clusterUri: RootClusterUrithanclusterUri: string.
Ah, those would still be typed, we wouldn't use string. It's just that because we cannot differentiate between root and leaf URIs, we'd have a single ClusterUri type that accepts both. So instead of clusterUri: RootClusterUri you'd have rootClusterUri: ClusterUri. And as Michael said, I think in those cases the name of the variable should be rootClusterUri no matter what the type is. ;)
However, we could make the same argument about cluster URIs vs resource URIs. This code works:
const serverUri: RootClusterServerUri = '/clusters/foo/servers/bar'
const uri: RootClusterUri = serverUri
const leafServerUri: LeafClusterServerUri = '/clusters/foo/leaves/baz/servers/bar'
const leafUri: LeafClusterUri = leafServerUriBut I wouldn't want to remove the distinction between clusters and resources in types simply because we can't prevent resource URIs being passed as cluster URIs.
So yeah, maybe let's keep it as it is for now and see how it works out. Refactoring this will be super easy anyway.
ravicious
force-pushed
the
ravicious/typed-uris
branch
from
1c81388 to
9125457
Compare
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
I'm going to take another look at this PR on Monday.
I tried to change just types where possible, I didn't even touch arg names just to reduce the possible bug surface here.
| options.kubeConfigRelativePath || | ||
| // We prepend the name with `rootClusterId/` to create a kube config | ||
| // inside this directory. When the user logs out of the cluster, | ||
| // the entire directory is deleted. |
There was a problem hiding this comment.
I moved this comment around because it was messing up syntax highlighting in my editor. ;f
avatus
left a comment
avatus
left a comment
There was a problem hiding this comment.
This one left my mind. Sorry about missing it. LGTM
I had problems with the new types and apps because I didn't create a separate type for app URI. So I decided to remove it all because it isn't used anyway.
This method wasn't used anywhere since #1203 got merged.
ravicious
force-pushed
the
ravicious/typed-uris
branch
from
9125457 to
ff0a751
Compare
|
Merged. Let me know if you encounter any issues in your open PRs. |
* Remove ClustersService methods related to apps I had problems with the new types and apps because I didn't create a separate type for app URI. So I decided to remove it all because it isn't used anyway. * Remove WorkspacesService.getWorkspacesDocumentsServices This method wasn't used anywhere since #1203 got merged.
* Remove ClustersService methods related to apps I had problems with the new types and apps because I didn't create a separate type for app URI. So I decided to remove it all because it isn't used anyway. * Remove WorkspacesService.getWorkspacesDocumentsServices This method wasn't used anywhere since #1203 got merged.
3 participants
No description provided.