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

types(runtime-dom): type for class property #8012

Merged
merged 18 commits into from
Nov 10, 2023

Conversation

basil-gor
Copy link
Contributor

Sometimes it's so sad that HTMLAttributes['class'] type is an any.
Developers have to "reinvent the wheel" or blindly hope that it will be added someday.

This is the day :)

I think it would be useful to add normal type and remove any.
I also added new tests for normalizeClass, some of them related to #7869.

@yyx990803
Copy link
Member

Can you also add type-related test cases here? https://github.com/vuejs/core/blob/main/packages/dts-test/tsx.test-d.tsx

@basil-gor
Copy link
Contributor Author

Can you also add type-related test cases here? https://github.com/vuejs/core/blob/main/packages/dts-test/tsx.test-d.tsx

Sure. Done ✅

export type ClassValue =
| undefined
| string
| Record<string, boolean>
Copy link
Member

Choose a reason for hiding this comment

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

Technically the Record value can be any truthy/falsy value.

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, you want me to extend the possible accepted values in the Record type to also accept truthy/falsy types for maximum backward compatibility with older versions of Vue?

For example:

type Truthy = true | string | number | {} | any[];
type Falsy = false | 0 | -0 | 0n | '' | null | undefined | NaN;

and result record type will be:

Record<string, Truthy | Falsy>

Copy link
Member

Choose a reason for hiding this comment

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

Not really, what I mean is the value should technically allow any since any value in JS is either truthy or falsy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I understand what you mean.
So, then there are two possible solutions:

  1. Try to formalize it something like this:
type Falsy = false | 0 | -0 | 0n | '' | null | undefined | typeof NaN;
type Truthy<T> = T extends Falsy ? never : T;

export type ClassValue =
  | undefined
  | string
  | Record<string, Falsy | Truthy<unknown>>
  | Array<ClassValue>
  1. Just set the unknown type (or any at the very least):
export type ClassValue =
  | undefined
  | string
  | Record<string, unknown>
  | Array<ClassValue>

I think the second option will be easier and more priority, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yyx990803 I'm sorry to bother you, but could you please rerewiew the changes? It's been two months since the last activity here. Thanks!

@vercel
Copy link

vercel bot commented Apr 14, 2023

Someone is attempting to deploy a commit to the vuejs Team on Vercel.

A member of the Team first needs to authorize it.

@sxzz sxzz added the ready to merge The PR is ready to be merged. label Aug 13, 2023
@sxzz sxzz changed the title Type for HTMLAttributes['class'] property types(runtime-dom): type for class property Sep 1, 2023
@github-actions
Copy link

github-actions bot commented Sep 1, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 86.4 kB 32.9 kB 29.7 kB
vue.global.prod.js 132 kB 49.6 kB 44.5 kB

Usages

Name Size Gzip Brotli
createApp 48 kB 18.9 kB 17.2 kB
createSSRApp 51.2 kB 20.2 kB 18.4 kB
defineCustomElement 50.4 kB 19.7 kB 17.9 kB
overall 61.3 kB 23.7 kB 21.6 kB

@vercel
Copy link

vercel bot commented Oct 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated (UTC)
sfc-playground ⬜️ Ignored (Inspect) Visit Preview Oct 3, 2023 6:21am

@yyx990803 yyx990803 merged commit 46e3374 into vuejs:main Nov 10, 2023
11 checks passed
@haoqunjiang
Copy link
Member

yyx990803 added a commit that referenced this pull request Nov 13, 2023
reverts #8012 due to breakage in downstream types
@basil-gor
Copy link
Contributor Author

What should I do now? 🙃
Go to the vuetify and fix the falls, and only then this PR will be merged again? What about backward compatibility?
So, what do you recommend me to do?

@yyx990803
Copy link
Member

I don't think we can land this change, honestly. Vuetify is just one of the examples of how downstream projects can be affected - there could be many more cases (e.g. in private production projects) that use similar types. We can't just go out there and help them fix it.

In a way, v-bind:class is designed to handle any possible value type, so the value of adding type checks here is somewhat limited. IMO it might not be justify the breakage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge The PR is ready to be merged. scope: types
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants