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

Update Vectorize types for GA release #2421

Merged

Conversation

ndisidore
Copy link
Contributor

Overview

This updates the types for Vectorize ahead of the GA release.
We have intentionally moved the "current" Vectorize Index to a legacy type, promoting the "new" index to the default VectorizeIndex such that it becomes the standard going forward.

This is strictly a types change and sister to
cloudflare/workers-sdk#6252

* Here, `mutationId` is the identifier for the last mutation processed by Vectorize.
*/
interface VectorizeVectorMutationV2 {
/* The identifier for the last mutation processed by Vectorize. */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, this previous comment was incorrect - each mutation gets a unique mutation id specific to the operation

Copy link
Contributor

Choose a reason for hiding this comment

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

The new comment is more accurate description of a mutation id.

@DaniFoldi
Copy link
Contributor

Hi @ndisidore,

I'm really excited for Vectorize GA, it has great potential. However, I have a few concerns about this PR, which I hope you don't mind me raising, in order of most to least important:

  • The type of VectorizeVectorMutation would suddenly change for all users in a non-backwards-compatible way. If my guess is correct, the behavior of existing v1 datasets wouldn't suddenly change, and therefore the types of my current worker would become out of sync with the actual runtime. See my idea below.
  • This removes the VectorizeMutationV2 interface, which was available in previous workers-types releases, even outside of experimental (if it were only in experimental, this wouldn't be an issue at all). Here's the type in the currently latest release with 2023-07-01 type compat date: https://workers-types.pages.dev/4.20240718.0/2023-07-01/#VectorizeVectorMutationV2. It's very unlikely that anyone external would be using it today, however removing it in a minor is a semver violation, probably better to add the @deprecated jsdoc flag so tsc warns users about it, and remove it in the next major.
  • I'd also add @deprecated to all ...legacy types, so users are warned if they are using them.

My suggestion for keeping it one type is to use a generic with a default to distinguish between the types. Here's a link to a quick example: https://www.typescriptlang.org/play/?#code/C4TwDgpgBAsgrsAhsAlgewHYDUCMUC8UA3lHAM4QBOAkgCYBcUZwlKGA5lAL4BQoksBMnTYATAWKkKlAHKIAthEbNWHANxQUDJizadefcNHhJUmADxYqZEVAgAPYBAy0yUAOS53UAD4esot6EnjjuAHwSVpQ2mHaOzq7+oVAA-IKmIrhQjCbCmAE8PLQQAMYANoiU0CWYzFDyODlCZhg8APRtUF3dPV0AeimFxeWV1bXA9aJNGRYh4e2dvUsDQ6UVVVA1GHXyAMzTeRjmnoFhC0vLg0A

The key points are: if nothing changes, the old one is used, to avoid changing existing v1 indexes. Users could be told to use either the type with a generic or the V2 override. Wrangler will probably have a way to detect which type a binding should be and it can emit the correct type.

@ndisidore ndisidore force-pushed the nathan/vectorize-ga-types-rev2 branch 2 times, most recently from 0c4fc3e to 200065f Compare July 22, 2024 17:06
@ndisidore
Copy link
Contributor Author

ndisidore commented Jul 22, 2024

Hey @DaniFoldi 👋
Thanks for you interest and also your feedback!

The generics approach you linked to is a fun one and is certainly something we can consider. We'd looked at something vaguely similar but opted against it primarily because with that, the "naked" type, while it wouldn't be breaking, would default users to the depreciated legacy binding format. This would likely be the case for the forseeable future lest we push the breaking change problem to a later date.
For better or worse, workers types doesn't follow semver format so its tough to do something like this properly.
The nice thing there is that types is an npm package, so if you avoid updates, nothing changes.

For most folks, this should be a 1 line change: /s/VectorizeIndex/VectorizeIndexLegacy/

Given the above, that Vectorize is in beta, and that this change is strictly for types, we've decided that we're okay with a breaking change here, but we still welcome (and are gathering) feedback. It should be stressed that once this enters GA, our approach here would look very different.

@DaniFoldi
Copy link
Contributor

Hey @ndisidore 👋,

Thanks for the response, I appreciate your time!

While I understand your reasoning, I firmly believe that the workers-types package does follow semver (and I believe the original intention was to expose unsafe types only in the experimental entrypoint, see DO storage sqlite methods for example, so to me this counts as a stable part of a stable package) and that is also why EmailExportedHandler still has a type name that is inconsistent with others (even though it is still in beta), and why Request has weird generics (see Brendan's description why: #483 (comment)).

I hope that this api type change will be communicated to users via docs (and I would also add an inline jsdoc mentioning the legacy types for anyone still using v1 and seeing errors after they update) and changelog so users are aware of the changes they need to make. Going forward it could also help if you marked beta types (or all types of beta products that are available in "stable" workers-types entrypoints) with @alpha, @beta or @experimental tags (I've been using jsdoc and tsdoc interchangeably, even though these would be tsdoc in this case).

@ndisidore ndisidore force-pushed the nathan/vectorize-ga-types-rev2 branch from 200065f to d0dc999 Compare July 23, 2024 18:26
@ndisidore ndisidore force-pushed the nathan/vectorize-ga-types-rev2 branch 2 times, most recently from df9165c to ae26ca9 Compare July 23, 2024 19:17
types/defines/vectorize.d.ts Show resolved Hide resolved
types/defines/vectorize.d.ts Outdated Show resolved Hide resolved
@ndisidore ndisidore force-pushed the nathan/vectorize-ga-types-rev2 branch from ae26ca9 to 5b48b9d Compare July 24, 2024 12:41
types/defines/vectorize.d.ts Outdated Show resolved Hide resolved
types/defines/vectorize.d.ts Outdated Show resolved Hide resolved
@ndisidore ndisidore force-pushed the nathan/vectorize-ga-types-rev2 branch from 5b48b9d to 22829e1 Compare July 24, 2024 14:13
@Frederik-Baetens Frederik-Baetens merged commit 4f8ddda into cloudflare:main Jul 24, 2024
9 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.

6 participants