refactor: migrate Zod to pure TS w/ JSDoc, improve type safety and simplify parsing #217

ssalbdivad · 2024-10-15T17:39:28Z

Description

This PR migrates the repo off of Zod schemas toward pure TS with JSDoc annotations that can eventually be used to extract metadata for display in docs.

Generally the external behavior should be identical, with obvious exceptions where schema entry points are no longer available etc., so this would constitute a breaking change for consumers relying on those entrypoints or whose inputs may no longer be valid with new type safety around video/image options.

I've also updated all the tests to .ts so we can be sure the types for the API are working as we expect going forward.

Although all the tests are passing, the doc generation issue remains unsolved, causing the repo-wide build to fail, so that will need to be updated to use a JSDoc parsing tool. However, it should be quite straightforward to wire that up in place of the previous Zod-embedded metadata.

For now, I'm opening this against the beta branch so the required JSDoc parsing logic can be added and other changes can be experimented with before broader consumption.

Issue Ticket Number

N/A

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Fix or improve the documentation
This change requires a documentation update

Checklist

(Still a draft, need to figure out how to work around missing schemas in build)

I have followed the contributing guidelines of this project as mentioned in CONTRIBUTING.md
I have created an issue ticket for this PR
I have checked to ensure there aren't other open Pull Requests for the same update/change?
I have performed a self-review of my own code
I have run tests locally to ensure they all pass
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes needed to the documentation

Baroshem

Really amazing work did there! I like the new approach much better and cant wait for it to be released so that I could check if the issue with Zod could finally be solved in Nuxt Cloudinary.

I have left few comments/suggestions for you to see but overall, a solid work! Kudos!

Baroshem · 2024-10-18T10:46:02Z

packages/url-loader/src/lib/plugin.ts

+>(
+  def: PluginDefinition<asset, when, name, options>
+): TransformationPlugin<asset, when, name, options> =>
+  ({ strict: false, ...def }) as never;


question: is this never intented?

as never is just a slightly cleaner/safer way than as any to allow an assignment that TypeScript doesn't understand is safe.

I see a lot of people do something like as TransformationPlugin<asset, when, name, options>, but in the end you're just adding a lot of complexity for no additional safety.

Baroshem · 2024-10-18T10:51:52Z

packages/url-loader/src/plugins/underlays.ts

-        if (!success) {
-          if (process.env.NODE_ENV === "development") {
-            console.warn(`Invalid flag ${flag}, not applying.`);
-          }
-          return;
-        }


question: shouldn't we maintain this functionality? I think it is missing from a new approach

I removed this based on feedback from @colbyfayock that we should be able to trust the input types to the functions themselves rather than performing runtime validation, which I think make sense if these flags are being invoked from an API directly.

If that's not the case, it would be easy enough to refactor those union definitions to tuples and then extract them back out to see if they're in the list, but in general I think the reasoning is good that if you have a typed API, you shouldn't need to revalidate all your inputs.

i dont feel particularly strongly one way or another here. i always thought that providing more console.logs or throws that move errors closer to the developer could be helpful but not at the expense of trying to maintain something like zod, so moreso if there is good opportunity i think we should leverage it but not for any kind of significant overhead. this example was using zod, so it would need refactoring

@Baroshem any thoughts around that?

Baroshem · 2024-10-18T10:55:32Z

packages/url-loader/src/plugins/version.ts

@@ -1,22 +1,20 @@
-import { z } from "zod";
-import type { TransformationPlugin } from "../types/plugins.js";
+import { plugin } from "../lib/plugin.js";


question: is this import correct? I can see that this file is plugin.ts in the file tree

Yeah, that is the default method TS recommends to do modern ESM imports in Node. Always a bit of a point of confusion/contention :P

It's actually more viable now to do .ts as of 5.7, but .js is the more standard approach (also not changed in this PR).

Baroshem · 2024-10-18T10:57:18Z

packages/url-loader/src/lib/utils.ts

@@ -0,0 +1,38 @@
+export const isArray: (data: unknown) => data is ReadonlyArray<unknown> =


suggestion: Maybe it would be worth adding a JSDoc here explaining why this utility was created as at first glance it may not be understanadable and devs would need to jump here to understand why normal isArray function was not used.

Makes sense, I'll add that. Array.isArray sucks for a couple reasons:

Narrows the type to any[]

Doesn't work with readonly arrays

thats interesting - can't help to wonder if there's any kinda native workaround / future spec that'll address that kinda thing. perhaps it wasn't the main intent, but i used to always use Object.keys but i learned that Object.entries is better for TS. wonder if there's an equivalent either now or in the future? but maybe there's not much additional value that could be provided so its not worth the effort?

Array.isArray would be much safer if it was defined like the version I implemented, but at this point it would be too big of a breaking change for TS since so much code is implicitly relying on that any ☠️

Luckily, there's no runtime overhead here as it's just a redeclared type with a reference to the original method.

Object.keys and Object.entries are a bit different. In those cases, TS has opted for the safe option by inferring a conservative type, but that isn't necessarily what you want if you have an object with known keys and want the narrowed types. That's what the entriesOf utility I added can help with.

cool thanks david

colbyfayock · 2024-10-18T18:10:39Z

lets get this into a beta branch!

# [@cloudinary-util/util-v5.0.0-beta.1](https://github.com/cloudinary-community/cloudinary-util/compare/@cloudinary-util/util-v4.0.0...@cloudinary-util/util-v5.0.0-beta.1) (2024-10-18) ### Features * migrate Zod to pure TS w/ JSDoc, improve type safety and simplify parsing ([#217](#217)) ([f605f26](f605f26))

github-actions · 2024-10-18T18:13:09Z

🎉 This PR is included in version @cloudinary-util/util-v5.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [@cloudinary-util/url-loader-v6.0.0-beta.1](https://github.com/cloudinary-community/cloudinary-util/compare/@cloudinary-util/url-loader-v5.10.5...@cloudinary-util/url-loader-v6.0.0-beta.1) (2024-10-18) ### Features * migrate Zod to pure TS w/ JSDoc, improve type safety and simplify parsing ([#217](#217)) ([f605f26](f605f26))

github-actions · 2024-10-18T18:22:03Z

🎉 This PR is included in version @cloudinary-util/url-loader-v6.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [@cloudinary-util/types-v2.0.0-beta.1](https://github.com/cloudinary-community/cloudinary-util/compare/@cloudinary-util/types-v1.5.11...@cloudinary-util/types-v2.0.0-beta.1) (2024-10-18) ### Features * migrate Zod to pure TS w/ JSDoc, improve type safety and simplify parsing ([#217](#217)) ([f605f26](f605f26))

github-actions · 2024-10-18T18:22:24Z

🎉 This PR is included in version @cloudinary-util/types-v2.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

ssalbdivad added 24 commits October 7, 2024 13:05

refactor

55da0f5

continue translating schemas

529cfcd

begin working on namespaces per-file

0958857

more

0328386

continuu

f17fcd9

work on qualifiers

6f247f0

underlays

e2dcf59

continu

151171e

continue simplifying parsing, rename with plugin suffix

62ec0d4

add node fetch type

6493c87

continuu

ea6e5c9

55

e6ddc3e

simplify more types

b6c0c39

continuu

89648b4

continue cleaning up

5267ab6

fix last type error

a38cd84

dependent on assetType

b7d096e

remove outdated build script

b948f3e

merge main

7433c35

being converting tests to TS

367565c

convert tests to ts and fix

81c04d2

convert remaining unsafe Array.isArray calls to isArray from util

217568b

remove zod from deps and schemas as an entrypoint

33b1a1b

Merge branch 'main' into pure-ts

0a61b8e

ssalbdivad mentioned this pull request Oct 15, 2024

refactor: migrate Zod to pure TS w/ JSDoc, improve type safety and simplify parsing #214

Closed

12 tasks

ssalbdivad added 3 commits October 15, 2024 14:11

move new utils back to url-loader

80996f9

add validatePlugins type

61b2029

cleanup association between options and plugins

4dd624b

Baroshem reviewed Oct 18, 2024

View reviewed changes

add jsdoc for

fdaf28d

colbyfayock merged commit f605f26 into cloudinary-community:beta Oct 18, 2024

github-actions bot added the released on @beta label Oct 18, 2024

colbyfayock mentioned this pull request Oct 25, 2024

v7.0.0 cloudinary-community/next-cloudinary#523

Open

colbyfayock mentioned this pull request Nov 1, 2024

Cleanup & Migration to Refactored Util Package cloudinary-community/astro-cloudinary#63

Merged

		@@ -0,0 +1,38 @@
		export const isArray: (data: unknown) => data is ReadonlyArray<unknown> =

refactor: migrate Zod to pure TS w/ JSDoc, improve type safety and simplify parsing #217

refactor: migrate Zod to pure TS w/ JSDoc, improve type safety and simplify parsing #217

Uh oh!

Conversation

ssalbdivad commented Oct 15, 2024

Description

Issue Ticket Number

Type of change

Checklist

Uh oh!

Baroshem left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssalbdivad Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colbyfayock commented Oct 18, 2024

Uh oh!

github-actions bot commented Oct 18, 2024

Uh oh!

github-actions bot commented Oct 18, 2024

Uh oh!

github-actions bot commented Oct 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ssalbdivad Oct 18, 2024 •

edited

Loading