refactor: remove applyWhen in favor of explicit props overlap check with alwaysApply exception #226

ssalbdivad · 2024-10-25T20:55:03Z

Description

Simplify applyWhen to just check for presence of props from the new props key as discussed with @colbyfayock, add an alwaysApply key that can be used for exceptions like SanitizePlugin that should always run regardless of the presence of associated keys.

Issue Ticket Number

Fixes #<ISSUE_NUMBER>

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

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

colbyfayock · 2024-10-27T18:32:54Z

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

    streamingProfile: true,
  },
  apply: (asset, opts) => {
-    if (typeof opts.streamingProfile === "string") {


so my only concern here is that this is explicitly checking for a string, but it doesnt seem that the other side is checking the data type, so someone could pass in any truthy value and it would pass it along to the transformation?

This kind of falls under the previous conversation we had about runtime validation. Since they are providing typed input (the streamingProfile prop already enforces that the value is a string), passing true or something should be a type error.

would that be a type error if they're not using Typescript?

You can enable checkJs even if they're writing JS.

Based on our previous conversation, my impression was that we're assuming that people are using the API according to the types or the docs, i.e. not passing garbage. I think this is a good assumption when building a typed API, as otherwise you add tons of bloat for error handling that should never really occur based on the contract in general.

If there was a reason to expect untyped input/lots of mistakes, we'd kinda be back to square one wanting a runtime validator like Zod 😛

is checkJs a user setting or library setting? guessing user setting? so idk if thats a good assumption

i think its more likely people will be using typescript in a Next.js app these days given its a default but i think there still need to be handling, but i guess my impression of the conversation wasn't whether or not to include the runtime validation i already had, moreso not using Zod for it, which i think was only being used in one or two places

seems like its just regressing things a hair for negligible value since it doesnt rely on zod? most of the checks that were there were simpler checks and most relied on specific code paths as opposed to simple validation

It is a user setting, but what I'm describing is really a much broader principle in developing APIs that if someone is using your API in a way that isn't consistent with the contract, it's not your responsibility to check for every one of the infinite ways it could theoretically be misused.

Even if that's not enforced through TypeScript, your API docs still define a contract where the only allowed value type there would be a string, and the behavior would probably reasonable-ish if for some reason e.g. a number were provided- it would likely be interpolated into the URL string the same way it would have been if it were provided as a numeric string.

In fact, I'd say if I did pass a number, that behavior would be more intuitive to me than just ignoring it altogether which I think is what occurs as is?

Anyways, I'm happy to go back through the PR and reintegrate all the runtime checks, but I just want to flag that I don't think it's the right approach for a library like yours.

i agree that its not my responsibility but i do believe if i can provide guide rails that will have negligible impact on the code/project but have a beneficial impact on their developer experience, i think its worth the effort, like this isn't really going to break the bank in terms of performance but it can help avoid broken experiences

what will happen is instead of getting a broken video because of an improper value (like a number), which can be very difficult to debug through Cloudinary (like not everyone knows about x-cld-error header), it will not add this parameter to the URL maintaining the video still in a working experience

an enhancement would be to warn of the usage in development mode like i do in the if (!getFormat(defaultImage)) { below but i opted not to specifically do that in every instance

Okay I will add those checks back in!

colbyfayock · 2024-10-27T18:33:36Z

packages/url-loader/src/plugins/default-image.ts

  apply: (asset, opts) => {
    const { defaultImage } = opts;

-    if (typeof defaultImage === "string") {


similar question

colbyfayock · 2024-10-27T18:34:41Z

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

-  apply: (cldAsset, options) => {
-    if (options.enhance) {
+  apply: (cldAsset, opts) => {
+    if (opts.enhance) {


why would this be left in then?

Because the type here is a boolean, so the value false would be legal, so we have to check for that.

colbyfayock · 2024-10-27T18:37:21Z

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

+  apply: (cldAsset, opts, ctx) => {
+    const { seoSuffix } = opts;

-    if (typeof seoSuffix === "string") {


similar about string check

packages/url-loader/tests/plugins/remove-background.spec.ts

colbyfayock · 2024-10-27T18:39:02Z

here's the build error from the logs in case its not visible

ssalbdivad · 2024-10-29T16:26:51Z

@colbyfayock My bad on the overlapping plugin props issue there, at least there was a clear type error that I definitely should have noticed :P

@colbyfayock

…lwaysApply exception (#228) # Description  Simplify applyWhen to just check for presence of props from the new props key as discussed with @colbyfayock, add an alwaysApply key that can be used for exceptions like SanitizePlugin that should always run regardless of the presence of associated keys. This is a rebased version of #226. ## Issue Ticket Number Fixes #<ISSUE_NUMBER>   ## 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  - [ ] I have followed the contributing guidelines of this project as mentioned in [CONTRIBUTING.md](/CONTRIBUTING.md) - [ ] I have created an [issue](https://github.com/colbyfayock/cloudinary-util/issues) ticket for this PR - [ ] I have checked to ensure there aren't other open [Pull Requests](https://github.com/colbyfayock/cloudinary-util/pulls) 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

colbyfayock reviewed Oct 27, 2024

View reviewed changes

packages/url-loader/tests/plugins/remove-background.spec.ts Show resolved Hide resolved

ssalbdivad closed this Oct 31, 2024

ssalbdivad force-pushed the beta branch from 8d3b70a to 23e9afd Compare October 31, 2024 18:52

ssalbdivad mentioned this pull request Oct 31, 2024

refactor: remove applyWhen in favor of explicit props overlap check with alwaysApply exception #228

Merged

12 tasks

refactor: remove applyWhen in favor of explicit props overlap check with alwaysApply exception #226

refactor: remove applyWhen in favor of explicit props overlap check with alwaysApply exception #226

Uh oh!

Conversation

ssalbdivad commented Oct 25, 2024

Description

Issue Ticket Number

Type of change

Checklist

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 31, 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!

Uh oh!

colbyfayock commented Oct 27, 2024

Uh oh!

ssalbdivad commented Oct 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ssalbdivad Oct 31, 2024 •

edited

Loading