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

Added support for Array<string | number | boolean | null | undefined | Record<string, boolean>> in classList #308

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

samuelgozi
Copy link

@samuelgozi samuelgozi commented Jan 18, 2024

I saw that there was a somewhat similar PR #88, however this PR differs from it by not trying to support nested objects inside an array.

Edit: I added support for one level of nested objects into the array. I updated the description to reflect that.

I purposely didn't want to change too much since I wanted this change to be small and incremental. Perhaps in the future, if I see that nested objects inside an array is indeed a nice API I might volunteer to add it.

This PR is only additive, and doesn't change any existing behavior.

The specific implementation in this PR is very short and doesn't change the existing logic at all for the existing syntax for objects. However it adds one more loop for classList Arrays in order to convert them into an object, which is then treated the same as it does before this PR. If you don't like this approach I can offer an alternative.

I opened a discussion about this here with some examples:
solidjs/solid#2036

I also added tests with all relevant examples.

You can play with and test the implementation here (open the console):
https://stackblitz.com/edit/typescript-jbx9jn

General logic overview

classList now supports two types of arguments:

  1. Record<string, boolean> (Same as before)
  2. Array<string | number | boolean | null | undefined | Record<string, boolean>>

Objects are not allowed to have Arrays or nested objects, only booleans.
Arrays can have objects with booleans, but not arrays, nor objects with objects.
This keeps implementation and logic easy to reason about.

When classList is called with an array, it will convert the array into an object while skipping all false values except 0.
This has the side effect of leaving true as an object key, but that is also what happens in VueJS, so I consider that OK.

Edit: Now if the array contains objects, it will "flatten" them in the order they were passed. The behavior is like the spread operator.

Changes are then handled as if it was a regular object, so it essentially works exactly like passing an object with all values set to true.

Examples

Strings

const arr = ['one', 'two', 'three']
// is equal to
const obj = {
  one: true,
  two: true,
  three: true
}

Booleans

const arr = ['a', false, 'c']
// is equal to
const obj = {
  one: true,
  three: true
}
const arr = ['a', true, 'c']
// is equal to
const obj = {
  one: true,
  true: true,
  three: true
}

Undefined

const arr = ['a', undefined, 'b']
// is equal to
const obj = {
  one: true,
  three: true
}

Null

const arr = ['a', null, 'b']
// is equal to
const obj = {
  one: true,
  three: true
}

Numbers

const arr = ['a', 0, 'b']
// is equal to
const obj = {
  one: true,
  "0": true,
  three: true
}

Objects in the array

const arr = ['a', 'b', 'c', { b: false, d: true }]
// is equal to
const obj = {
  a: true,
  b: true,
  c: true,
  b: false,
  d: true
}

// Which is equal to
const obj = {
  a: true,
  c: true,
  b: false,
  d: true
}

@samuelgozi
Copy link
Author

If this PR is approved I have no problem documenting it in the official Solid docs.

@samuelgozi
Copy link
Author

samuelgozi commented Jan 18, 2024

I thought about it and maybe in order to support objects in the array we can flatten nested object into the returned object from classListToObject.
It will essentially work like Object.assign, so it will be easy to reason about if multiple objects contain the same keys, and otherwise functionality still remains the same.

Then the type of ClassList changes to:

type ClassList =
  | Record<string, boolean>
  | Array<
      string | number | boolean | null | undefined | Record<string, boolean>
    >;

So nested objects inside objects are not allowed.

Edit: Added this capability and edited the PR description.

@ryansolid What do you think?

@samuelgozi samuelgozi changed the title Added support for Array<string | number | boolean | null | undefined> in classList Added support for Array<string | number | boolean | null | undefined > in classList Jan 18, 2024
@samuelgozi samuelgozi changed the title Added support for Array<string | number | boolean | null | undefined > in classList Added support for Array<string | number | boolean | null | undefined | Record<string, boolean>> in classList Jan 18, 2024
@lxsmnsyc
Copy link
Collaborator

This lacks the compiler optimization step for classList, might be needing that if we were to approve this

@samuelgozi
Copy link
Author

Can you give me some directions on how to do that, or where it is?

@ryansolid
Copy link
Owner

Yeah this doesn't look at the compiler side of things. It's possible we just need a heuristic to bail out of the optimization I suppose. Like if we see an array, or some of the other values.

I'm not going to lie classLIst's future is still up in the air. It is one of the things we know for sure that will be reviewed in 2.0. So this might drag a bit until we can figure out how to proceed.

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.

None yet

3 participants