QwikDev / qwik

Instant-loading web apps, without effort
https://qwik.dev
MIT License
20.76k stars 1.29k forks source link

[✨] - Built-in Conditionals #2678

Closed jdgamble555 closed 7 months ago

jdgamble555 commented 1 year ago

Is your feature request related to a problem?

Yes, JSX does not make conditionals easy. If statements need ternary or && operators, arrays need maps, and complex switch statements are not possible without creating custom components. JSX is just not as simple as say:

Describe the solution you'd like

In a perfect world, I would like to NOT use JSX for these condtional statements, and add the ability to do things like this mixed with JSX:

Similar to Svelte

{if me === true}
  <Header />
{/if}
{if you === 'him'}
  <Component1 />
{else if love === 'hate'}
  <Component2 />
{else}
  <Component3 />
{/if}
{if posts}
  {for posts as post, i, key=post.id}
    <Post {key} {post} />
  {/for}
{/if}

OR

Similar to Vue

<q-if={me === you}>
  <Component1 />
<q-else-if={boy === girl}>
  <Component2 />
</q-if>
<q-for={posts as post} index={i} key={post.id}>
  <Post {key} {post />
</q-for>

Describe alternatives you've considered

If that is not feasible to break JSX specs (not sure the exact spects), you may just need to write your own JSX components that would be a default part of every project.

Ideas:

I would probably write my own components like this:

<qk if={me === you}>
  <Component1 />   
</qk>
<qk switch={condtion}>
  <qk case={case1}>
    <Home />
  </qk>
  <qk case={cas2}>
    <Component1 />
  </qk>
  <qk default>
    <Default-component />
  </qk>
</qk>
<!-- default value for key is 'key' -->
<qk-for={posts} key={'key}'>
  {post => <Post {post} {key} />}
</qk-for>

<!-- honestly not sure this is much better than map -->

Under the hood should not use map, and should use for, as it has been proven to be faster than map.

Additional context

I understand that most programmers know JSX, so JSX is better for React users to learn Qwik, however... popularity should not be confused with facility. Svelte, by far, has mastered this. JSX just does not make basic conditionals and loops easy to use. Vue is second best.

If we have to stick with JSX, let's create the easiest to use jsx-compliant components to be the next Qwik standard, like SolidJS did. I could write my own, but I don't want to if there is a "qwik way" of doing things.

J

DustinJSilk commented 1 year ago

This would be cool!

Theres also a possible route that works more like the named slots which could reduce nesting:

<div q:if={me === you}>...</div>

<div q:switch={me === you}>
  <div q:case={case1}>...</div>
  <div q:case={case2}>...</div>
  // default
  <div q:case>...</div>
</div>
manucorporat commented 1 year ago

Funny enough JSX is the template system than less reinvents the control flow :D

The SolidJS, or even vue way could be easily implemented in user-land without changes.

Something to think about:

Interesting conversation! I am not against to make anything in our hand to make Qwik easier to use and get developers more productive

adam-r-kowalski commented 1 year ago

I really like how the Svelte syntax looks, but it is not extensible by users, meaning we cannot create additional control flow primitives that are not part of the core "language".

The advantage of something like Solid is that the control flow looks like any other component. You can always create new components, which expose different control flow structures if the ones that are "built-in" don't quite match what you want.

Having things like

// this is Solid
<Switch fallback={<p>{x()} is between 5 and 10</p>}>
    <Match when={x() > 10}>
        <p>{x()} is greater than 10</p>
    </Match>
    <Match when={5 > x()}>
        <p>{x()} is less than 5</p>
    </Match>
</Switch>

Look great and feel like they are part of the language and match the existing declarative approach, as opposed to another "kind" of language like

// this is Svelte
{if x > 10}
    <p>{x} is greater than 10</p>
{else if 5 > x}
    <p>{x} is less than 5</p>
{else}
  <p>{x} is between 5 and 10</p>
{/if}

which in my opinion while more readable looks like a blend of two different languages, not a cohesive design.

DustinJSilk commented 1 year ago

An if-component is quite simple. To @manucorporat point, you could just save this in your own repository:

import { component$, Slot, Signal } from '@builder.io/qwik'

export const If = component$((props: { value: any }) => {
  return <>{props.value && <Slot />}</>
})

export default component$(() => {
  const signal = useSignal(true)

  return <If value={signal.value}>...</If>
})

The switch is a little more difficult however, i haven't quite managed to get it working using slots

adam-r-kowalski commented 1 year ago

As long as all the branches are mutually exclusive then you could just stack many If next to each other and only the "active" one will show up. You might be able to do something with named slots to provide a form of else but I'm not sure thats a very pleasant API.

@manucorporat you mentioned you could do this without userland changes. Can you show how?

DustinJSilk commented 1 year ago

I found the tricky part is having an outer component pass context down to the inner Slot components, and allowing multiple Switch statements on a single page.

literalpie commented 1 year ago

Having the if/else or switch components could be helpful in some places where people run into the single-jsx-root linting rule.

jdgamble555 commented 1 year ago

To me, my biggest critique of JSX is when people pass HTML as a variable. This is ugly, odd, and confusing.

This may not seem bad:

<Show when={state.count > 0} fallback={<div>Loading...</div>}>
  <div>My Content</div>
</Show>

Then it gets more complicated:

<AuthCheck
  fallback={
    <Link passHref href="/enter">
      <button>πŸ’— Sign Up</button>
    </Link>
  }
>

or

<Show
  when={connectionStatus}
  fallback={
    <div className="border rounded bg-red-400 py-2 px-5">
      ❌ disconnected
    </div>
    }
  >
  <div className="border rounded bg-green-400 py-2 px-5">
    βœ… connected
  </div>
</Show>

This is not natural to me, and seems counterintuitive. IMHO, fallback html should be handled outside of the component using regular if / switch statements, not as a variable input itself.

Fallback Alternatives

What you're really saying is: if (state.user === undefined) or with switch, else {}. Adding the title fallback is disingenuous IMO, and adds an extra layer of confusion. This should be covered in regular use cases.

For example, you should have a separate loading variable to define if your app is loading. JS or TS can confuse null and undefined. You should also cover else use cases as a separate condition.

That being said, I don't think you can pass a component as a variable anyway in Qwik, as during my tests I get Code(3): Only primitive and object literals can be serialized. This gets rid of that option.


If

I personally like the If component like so:

export const If = component$((props: { case: boolean }) => {
    return <>{props.case && <Slot />}</>;
});
<If case={signal.value}>true if statement</If>

Switch

The switch, however, seems impossible since there is no way to loop through the children in Qwik jsx components like you can in React or Solid.

You could add the components as a prop, but I think this is a bad idea just like fallback is (see above). Qwik would need to add a way to support looping through the JSX to their core code, or passing JSX as variables. Probably not a good idea anyway.

Loops

We have the same problem as loops, as we can't pass components as a variable, and we can't loop through the children.

What to do?

So, to me, the Qwik way of doing things should copy its format for slots and use the q:if syntax that @DustinJSilk suggested above:

It would get rid of boilerplate, and be extremely simple to use. If we don't want to display a div (just like the fragment issue), you could use it just like q:template which Qwik already has:

<q:switch={state}>
  <q:if={state.loading === true}>
    <span>...loading</span>
  </q:if>
  <q:if={state.user !== null}>
    {user.name}
  </q:if>
</q:switch>

Or add it to the existing q:template:

 <q:template q:if={state.loading === true}>
  sumthing good
</q:template>

and with loops:

<q:for={items}>
  {(item, index) => (
    <Post {item} key={item.id} />
  )}
</q:for>  

Would be the Qwik-way,

J

adam-r-kowalski commented 1 year ago

I like it by my issue with a lot of these is "totality checking". With typescript if you use a switch statement, or an if statement, but don't cover all the branches, typescript will do it's best to warn you that there is a flow through the code that doesn't result in a return. Is there any chance we can retain the underlying checks that typescript provides on top of whatever syntax we choose?

Also I do think we might want to consider dropping the q: prefix as i'm not sure what extra additional information it adds. Is it just to know that it comes from qwik as opposed to something you wrote? Why does that matter? If it's so we can keep the name lowercase, well it's a component so I think people would expect it to be uppercase anyway right? Personally I think in the long run a shorter name like For Switch If is nicer then q:if or q:template q:if.

wmertens commented 1 year ago

Personally, I really dislike this :-)

Javascript is right there and has tooling and expressivity, and then these approaches take all that away and use different, more verbose and more limited syntaxes to achieve the same thing.

What's more, implementing this in userspace causes extra components to be created and evaluated, just for DX.

So while I understand some may really like this, I would like to ask that any solution does not come at a cost to non-adopters, nor to the end users.

I think the best way forward would be a Vite plugin that pre-processes the source, converting proposed syntax to regular JSX syntax.

jdgamble555 commented 1 year ago

JavaScript does, but JSX does not. Huge difference. You simply cannot express conditionals or loops without hacks. This is why every other framework except React has realized this.

Perhaps if Qwik were to implement something like this natively, it would not cause an extra component render under-the-hood.

I'm not partial to the method, just that a better way exists other than pure JSX.

Of course, this is IMHO.

J

jdgamble555 commented 1 year ago

Keuller MagalhΓ£es on discord had a solution to the For problem, by using props:

import type { FunctionComponent } from "@builder.io/qwik";
import { component$ } from "@builder.io/qwik";

export interface ForProps {
    each: any[] | null | undefined;
    fallback$?: FunctionComponent;
    item$: FunctionComponent;
}

export const For = component$<ForProps>((props) => {
    if (!props.each || props.each?.length < 1) {
        return (props.fallback$ ? props.fallback$({}, null) : null);
    }
    return (<>{ props.each?.map((item, idx) => props.item$(item, `${idx}`))}</>)
});

And you would use it like so:

<div class="card-list">
   <For each={people.value} 
        fallback$={() => (<div>No data found</div>)}
        item$={(item, key) => <Card key={key} name={item.name} />} />
</div>

However...

As @mhevery mentioned (and solving @wmertens's problem), we should NOT use component$, as a lite component would be better to solve the perf and styling problems. I modified the above code to work with lite components, added the index value, and generics.

// IF Component
export type Optional<T> = T | null | undefined;

export type IF = <T>(props: {
    case: boolean;
    children: JSX.Element | ((item: T) => JSX.Element);
}) => JSX.Element;

export const If: IF = (props) => <>{props.case && props.children}</>;

// FOR Component
export type FOR = <T, U extends JSX.Element>(props: {
    each: Optional<T[]>,
    fallback?: Optional<JSX.Element>
    item: (item: T, index: number) => U
}) => JSX.Element;

export const For: FOR = (props) => {
    return <>
        {props.each === undefined && props.fallback
            ? props.fallback
            : props.each
                ? props.each?.map((item, idx) => props.item(item, idx))
                : ''
        }
    </>
};

That being said, now that we can just use regular JSX components, that opens the door to use children, which opens the door to copy EXACTLY the same code that solidjs is using. It seems like the team is open to a PR on this, but doesn't want to take their own time to do this, as it is LOW priority for obvious reasons.

So, we need to figure out these things:

  1. Names of these two functions (three if we add switch): For, If, Show, Switch, Loop, Each, etc...
  2. Prototypes of these functions (what input do we actually want, need, etc)
  3. The actual code

I don't mind writing the code, if anyone has any suggestions maybe we could agree on?

Also, worth noting that SolidJS displays loading in both undefined and null cases. This may not be the desired outcome. undefined could be the case for loading, but null if there is no actual data after the data loads (besides an empty array). Not trying to over-think this, but just a thought.

Thoughts (I'm over-invested in this)?

J

keuller commented 1 year ago

@jdgamble555 I will make the changes suggested by @mhevery and also open a PR for that.

  1. I think the current names For and Show are pretty straightforward and intuitive (thanks solidjs for that)
  2. Open discussion
  3. I'm working on it

Regarding the desired outcome when the value is undefined to me makes sense.

jdgamble555 commented 1 year ago

I already made those changes above, added generics, and the index option. It still can be better. We also need an if/else or a switch.

I prefer using slot version than the input version of each. We can copy solid exactly, but I don't think it is good taste to do so.

Please post your code here 1st before you make a PR. Let's make it the best we can first.

Thanks,

J

keuller commented 1 year ago

@jdgamble555 below there is the code of these tags:

Show tag

import type { JSX } from "@builder.io/qwik/jsx-runtime";
import type { FunctionComponent, JSXNode } from "@builder.io/qwik";

export type ShowProps = {
    when: boolean | null;
    fallback?: FunctionComponent<() => JSXNode | JSX.Element>;
    outcome: FunctionComponent<() => JSXNode | JSX.Element>;
}

export const Show = (props: ShowProps): JSXNode | JSX.Element | null => {
    const { when, outcome, fallback } = props;
    if (when === undefined || (when === null && !fallback)) return null;
    if (when === null && fallback) return fallback({}, null);
    if (when) return outcome({}, null);
    return null;
};

usage

<Show when={toogle.value} 
    outcome={() => <Info message="Lorem ipsum dolores avec mi" />} 
/>

For tag

import type { JSX } from "@builder.io/qwik/jsx-runtime";
import type { FunctionComponent, JSXNode } from "@builder.io/qwik";

export type ForProps = {
    each: any[] | null;
    fallback?: FunctionComponent<() => JSXNode | JSX.Element>;
    item: FunctionComponent<() => JSXNode | JSX.Element>;
}

export const For = (props: ForProps): JSXNode | JSX.Element | null => {
    const { each, fallback } = props;
    if (each === undefined) return null;
    if (fallback && (each === null || each?.length < 1)) {
        return fallback({}, null);
    }
    return (<>{each?.map((item, idx) => props.item(item, `${idx}`))}</>);
}

For usage

<div class="card-list">
    <For each={people.value} 
        fallback={() => (<div>No data found</div>)}
        item={(item, key) => <Card key={key} name={item.name} />} />
</div>

Both tags are changed in order to use Lite Component instead of component$. Open for discussion.

jdgamble555 commented 1 year ago

You can edit and style your code with syntax highlighting with the correct color themes by using jsx or html or js etc. after your three backticks. It makes it easier for all of us to read πŸ™‚

So, I'm not sure what the difference here is besides the name? I had already fixed it and added some more syntax.

There is no reason to create a function outcome or item when we are using regular JSX syntax instead of $component. We can just pass the item as a child the same way Solid JS does. I would like to limit functional parameters for JSX personally, as I find it is not very readable (as I spoke of above), unless you see a good reason not to use it.

Also, fallback({}, null); will not compile in typescript, as you can't return {}. I'm not sure why you used functions here?

We also need a Switch component. We can just create the exact same components SolidJS uses if that is your prerogative. Does anyone else here have any opinions or suggestions?

I will see what I can come up with after work.

J

DustinJSilk commented 1 year ago

Nice. Heres a few of my thoughts

keuller commented 1 year ago

thanks @DustinJSilk for your considerations.

mhevery commented 1 year ago

So you either have Slot and and component$ OR children and no Slot. You can't have both.

So I think it should be:

So putting the above constraints together I see something like this:

<If then={() => <>true</>} else={() => <>false</>}/>
<If else={() => <>false</>}>{() => <>true</>}</If>
<For each={[]}>{(value) => <>{value}</>}</For>
keuller commented 1 year ago

@jdgamble555 @mhevery @DustinJSilk I refactored my code based on your suggestions and also created a simplest version of Switch.

export const If = (props: IfProps): JSXNode | null => { if (props.condition) { if (typeof props.then === 'function') { const fn = props.then as FunctionComponent; return fn({}, null); } return props.then; } else if (props.else) { if (typeof props.else === 'function') { const fn = props.else as FunctionComponent; return fn({}, null); } return props.else; } return null; }


### Usage
```html
<If condition={toogle.value}
    then={() => <Info message="Lorem ipsum dolores avec mi" />}
    else={() => <div>Disabled</div>} />

export const For = (props: ForProps): JSXNode | null => { const { each, fallback } = props; if (each === undefined) return null; if (fallback && (each === null || each?.length < 1)) { return fallback({}, null); } return (<>{each?.map(props.children)}</>); }


### Usage
```html
<div class="card-list">
    <For each={people.value} fallback={() => (<div>No data found</div>)}>
      {(item, key) => <Card key={key} id={item.id} name={item.name} />}
    </For>
</div>

export function Match(props: MatchProps): JSXNode { return props as unknown as JSXNode; }

export type SwitchProps = { fallback?: JSXNode | JSX.Element; children: JSXNode[]; }

export const Switch = (props: SwitchProps): JSXNode | JSX.Element | null => { if (props.fallback && (!props.children || props.children?.length < 1)) { return props.fallback; }

for(const match of props.children) {
    if (match.props.when) {
        return match.props.children;
    }
}

if (props.fallback) return props.fallback;
return null;

}


### Usage
```html
<div>
    <label>Choose Menu:</label>
    <select onChange$={(ev) => option.value = ev.target.value}  value={option.value}>
        <option value="1">Car</option>
        <option value="2">Airplane</option>
        <option value="3">Train</option>
    </select>
 </div>

 <Switch fallback={<div>Invalid option</div>}>
      <Match when={option.value === '1'}>
            <p>Option: 1 - Car</p>
      </Match>
      <Match when={option.value === '2'}>
            <p>Option: 2 - Airplane</p>
      </Match>
      <Match when={option.value === '3'}>
            <p>Option: 3 - Train</p>
      </Match>
 </Switch>
jdgamble555 commented 1 year ago

Nice work!

However, we can't use child components as @mhevery said -- it will execute eagerly.

A few things:

  1. Do you guys prefer the name condition over case?

    condition makes more sense, but case is shorter. I'm good either way, just thoughts.

  2. Why are you returning {}?

    This seems odd to me. I think I can clean up your typing, so I will have a look in a few hours.

  3. Since we can't use child components in the Switch either, it will unfortunately need to be a function.

    This includes child components in Switch AND If. I'm not sure that we even need a Match since If basically does the same thing.

I have some ideas so solve these problems, will post when I get home.

Thanks,

J

adam-r-kowalski commented 1 year ago

I really like the proposed For and Switch api here but the If I don't personally love.

I think the true branch should be a child so instead of :

<If condition={toggle.value}
    then={() => <Info message="Lorem ipsum dolores avec mi" />}
    else={() => <div>Disabled</div>} />

we have:

<If condition={toggle.value} else={() => <div>Disabled</div>}>
    <Info message="Lorem ipsum dolores avec mi" />
</If>

which at that point we might as well use Show instead

<Show when={toggle.value} otherwise={() => <div>Disabled</div>}>
    <Info message="Lorem ipsum dolores avec mi" />
</Show>

which honestly probably doesn't even need the otherwise as I find building out large components and passing them in is rather annoying. So maybe just:

<Show when={toggle.value}>
    <Info message="Lorem ipsum dolores avec mi" />
</Show>

If you need conditional logic then use the Switch Match

 <Switch>
      <Match when={toggle.value}>
            <Info message="Lorem ipsum dolores avec mi" />
      </Match>
      <Match when={true}>
            <div>Disabled</div>
      </Match>
 </Switch>

However, I actually think we should embrace the more functional Match, Case notation since we are writing declarative code

 <Match>
      <Case when={toggle.value}>
            <Info message="Lorem ipsum dolores avec mi" />
      </Case>
      <Case when={true}>
            <div>Disabled</div>
      </Case>
 </Match>

Since the Case when={true} is a common pattern as a "fallback" we can write

  <Match>
      <Case when={toggle.value}>
            <Info message="Lorem ipsum dolores avec mi" />
      </Case>
      <Otherwise>
            <div>Disabled</div>
      </Otherwise>
 </Match>
jdgamble555 commented 1 year ago

Hey @adam-r-kowalski,

The problem is we can't use child components with Qwik, as it will optimistically load the conditions (as @mhevery said, it will execute eagerly).

So the only other option would be to use functional inputs for conditionals and loops. I'm not a huge fan of them either, but it would be the "Qwik" way of doing things to get the best performance and no buggy code. If you have ideas on how to structure that, please post them here as well.

J

adam-r-kowalski commented 1 year ago

Sure: https://stackblitz.com/edit/qwik-starter-mbsnct?file=src/routes/index.tsx

I got lazy with some of the types sorry, I wanted to whip this up quickly. The concept is the important part.

jdgamble555 commented 1 year ago

Hey @adam-r-kowalski, we appreciate you taking the time, but we can't use children to generate conditionals (please read above again), as they may get loaded anyway. We have to use functions.

Here is what I came up with:

If

export type IF = (props: {
    case: boolean;
    then: JSX.Element;
    else?: JSX.Element;
}) => JSX.Element;

export const If: IF = (props) => <>{props.case ? props.then : props.else ?? null}</>;

Usage

<If
    case={true}
    then={<>True if statement</>}
    else={<>Else Statement</>}
 />

For

export type FOR = <T, U extends JSX.Element>(props: {
    each: T[];
    fallback?: JSX.Element;
    children: (item: T, index: number) => U;
}) => JSX.Element;

export const For: FOR = (props) => {

    const { each, fallback, children } = props;

    // case where each is undefined, so could be loading
    if (each === undefined && fallback) {
        return fallback;
    }

    // Qwik can only return one `<></>`, so looks weird
    return <>{
        // case where no array each
        (!each || !Array.isArray(each) || each.length === 0) ? null :

            // else loop through each item
            each.map((item, i) => children(item, i))
    }</>;
};

Usage

<For each={times} fallback={<>Loading...</>}>{(item, index) => <Test key={item.k} i={i} />}</For>

Switch

export type SWITCH = (props: {
    cases: JSX.Element[];
    default?: JSX.Element;
}) => JSX.Element;

export const Switch: SWITCH = (props) => {
    for (const c of props.cases) {
        if (c.props.case) { 
            return c.props.then;
        }
    }
    return props.default ?? <></>;
};

Usage

<Switch
    cases={[
      <If case={false} then={<>true switch</>} />,
      <If case={false} then={<>false switch</>} />
    ]}
    default={<>Default case</>}
/>

So a few things here:

  1. For the conditionals (If and Switch) I used functional input due to @mhevery's suggestion above.
  2. I covered ALL use cases for fallback without throwing errors. Eslint maybe better for that, but I can write the errors in there if that is a preference.
  3. Fallback on for is only loaded on undefined, not null in case the loaded data is null.
  4. We can't use find or something similar for the loops, as a real switch MUST execute in order.
  5. I could re-write If lite-component to be called Case or something similar, but it would do the exact same thing. Why re-write it?
  6. Some people seem to prefer Show over If. My suggestion is If, but whatever you guys decide.

Here is the code on Qwik Playground if you want to mess around -- you may need to refresh, as it can be buggy sometimes.

Let me know your thoughts,

J

mhevery commented 1 year ago

So you can use children but it must be a function like so:

<If condition={expr}>
  {() => <span>true</span>}
</If>
jdgamble555 commented 1 year ago

I honestly prefer my version with function inputs over children with functions... I feel like I would forget to add the anonymous function part. It is also not as clean IMHO. Although, you could add both options?

What does everyone else think?

J

DustinJSilk commented 1 year ago

What if we did some simple compile time work to wrap all children in functions with QwikCitys build? They are special components after all

otherwise @jdgamble555 ones are looking pretty good

keuller commented 1 year ago

Hey guys, actually what @mhevery said was "..NO component$ and rely on children." which means it isn't a real issue. The caveat is children executes eagerly. In the case of If I agree that using Function component is the best approach. TBH I really prefer Function components over JSXNode or JSX.Element. My suggestion is we consider the trade-offs here of each tag.

IMHO, keeping things as simple as possible is the secret, because in the end-of-the-day it is what we (developers) want to simplify our lives and keep the code understandable, straightforward and maintainable.

BTW I implemented it in my real project codebase and its working like a charm, no performance issues at all. πŸ˜ƒ

jdgamble555 commented 1 year ago

Good news is we mostly agree on For.

IMHO, keeping things as simple as possible is the secret, because in the end-of-the-day it is what we (developers) want to simplify our lives and keep the code understandable, straightforward and maintainable.

@keuller - Exactly, but this:

{() => <div> True condition </div>}

is 100% more complicated than this:

{<div>True condition</div>}

So, I'm not understanding why you prefer adding a function? It is like adding an extra locked door to your destination. You also made fallback a function, but not each, nor condition?

We should only go more complicated when necessary, otherwise keep it as simple and consistent as possible.

@DustinJSilk

What if we did some simple compile time work to wrap all children in functions with QwikCitys build? They are special components after all

@mhevery - Would this be possible to compile the JSX into a function at compile time?

J

keuller commented 1 year ago

@jdgamble555 great progress we agree about For πŸ˜„. Let move forward our discussion to If and Switch then.

I prefer the Function component instead of JSX.Element or JXSNode due to lazy loading processing. What I realised when I used JSX.Element and JSXNode is Qwik process it eagerly which in most case it is not desired behavior.

Regarding the readability of code, I agree {<div> ... </div>} is simpler than {() => <div> ... </div>}, it is a decision to make.

Regarding the attributes each and condition it should be simple primitives each -> Array and condition -> boolean. Also the fallback I prefer a function due to the principle of lazy loading.

I'm not a Qwik expert, but optimize-wise I believe the "Qwik mindset" is to use as many Function Component as we can, right? Internally I suppose Qwik execute function only when they are needed (lazy approach).

What do you think @mhevery @DustinJSilk ?

mhevery commented 1 year ago

I honestly prefer my version with function inputs over children with functions... I feel like I would forget to add the anonymous function part. It is also not as clean IMHO. Although, you could add both options?

TypeScript would help you here.

What if we did some simple compile time work to wrap all children in functions with QwikCitys build? They are special components after all

My previous experience tells me that this is creates too much magic which is very hard to debug as you don't expect the transformation. Our philosophy is that the compiler should do absolutely bare minimum which can't be done anywhere else.

@mhevery - Would this be possible to compile the JSX into a function at compile time?

As mentioned above, I don't think this is a good idea in terms of complexity, understanding, and surprises.

The reason why early execution matters is:

<If condition={expr}>
  <span>{expr.foo}</span> <!-- If expr==null this will throw an error because it is evaluated regardless of <If condition> -->
</If>
adam-r-kowalski commented 1 year ago

I do think perhaps a compiler based approach might be best. Maybe we should separate out the ideal API from how it's implemented internally. Personally I think using either the function components is more complicated DX then what many developers are expecting especially with the rise of Solid, Svelte, etc. I appreciate the fact that Qwik fundamentally changes the game with how little JavaScript it's able to ship, and so we need to work around those constraints, but to keep the attention of developers who may not care about that, the DX needs to be great, and they won't understand why they need to wrap components in functions.

Once we figure out the "right" DX we can figure out what the "correct" implementation should be to get the correct performance, loading semantics, etc. Maybe we need a for$ if$ switch$ component that cannot be implemented in user land, but can give people the appropriate semantics but maintain the syntax people want.

I'm not saying any of these syntaxes people have proposed are good or bad. I'm just saying that we should not constrain the API based on the limitations of whats currently possible to implement with the exposed APIs. These are components people will use potentially for years, so it's worth investing in the tooling to make the DX exactly what we want.

jdgamble555 commented 1 year ago

I prefer the Function component instead of JSX.Element or JXSNode due to lazy loading processing.

@keuller - so it is not a "preference," there is a reason to the madness. Reason and logic I understand. πŸ˜„

The reason why early execution matters is:

@mhevery - I was aware that children created early execution, but I was not aware that input JSX runs early.

For

@keuller - Do you mind fixing your code so these types are correct. You can't really pass {} as I said earlier. Also, you're showing fallback when there is null instead of undefined, which is the opposite of what we want. That way we can knock out the For. Also, make sure we have access to the index variable.

The goal here would be to avoid TS errors, any, and uknown where possible, and use generics T or U if necessary. I can look at it later after work if you need help.

for

If

So which version do we like here?

1

<If
    condition={expr}
    then={() => <>True condition</>}
    else={() => <>Else Statement</>}
 />

2

<If condition={expr} else={() => <>Else statement</>} >
  {() => <>True Condition</>}
</If>

I am bias, but I kind of like my version as inputs better in this particular case. It would be odd to me to put the else before the then. It also seems like you guys prefer condition here over case, which is fine with me. We could allow both (if there is no then, take the child), but that could be overly complicated.

Switch

On Switch, I would probably go with @keuller's case, except change fallback to default. when could be changed to where or condition, but not a big deal.

<Switch default={() => <div>Other option</div>}>
    <Case when={predicate}>
        {() => <div> .. </div>}
    </Case>
    <Case when={predicate}>
        {() => <div> .. </div>}
    </Case>
</Switch>

So far, it looks like I have lost in all rounds due to constraint issues πŸ˜…, but I do want to make sure we have clear and clean code before we come up with a final version :)

Does anyone else have any input?

J

jdgamble555 commented 1 year ago

I do think perhaps a compiler based approach might be best. Maybe we should separate out the ideal API from how it's implemented internally. Personally I think using either the function components is more complicated DX then what many developers are expecting especially with the rise of Solid, Svelte, etc. I appreciate the fact that Qwik fundamentally changes the game with how little JavaScript it's able to ship, and so we need to work around those constraints, but to keep the attention of developers who may not care about that, the DX needs to be great, and they won't understand why they need to wrap components in functions.

Once we figure out the "right" DX we can figure out what the "correct" implementation should be to get the correct performance, loading semantics, etc. Maybe we need a for$ if$ switch$ component that cannot be implemented in user land, but can give people the appropriate semantics but maintain the syntax people want.

I'm not saying any of these syntaxes people have proposed are good or bad. I'm just saying that we should not constrain the API based on the limitations of whats currently possible to implement with the exposed APIs. These are components people will use potentially for years, so it's worth investing in the tooling to make the DX exactly what we want.

I 100% agree with you here as I said above. Vue, Angular, and Svelte do this amazingly.

The problem is that Qwik uses JSX in the first place, which I personally do not like. The apparent pro for using JSX is that it is close to JS. However, we are writing more HTML than JS. It is DX vs Complexity problem here. I believe we can have both. That is my argument. I am also pro "compiler magic." I don't understand why this is a negative thing.

But it seems this is low priority for Qwik, and they don't want to add any internal features to make this simpler, so we are where we are. I think they don't want to change fundamentals in Qwik, while a few JSX importable components doesn't change much. My argument is that we should.

I still prefer my <q> version above.

So, better JSX than no components IMHO. I love Qwik, and they did an amazing job building it.

J

adam-r-kowalski commented 1 year ago

The other option then is rather then us proposing the API that qwik itself will use, since we can implement these all in user land we can have many libraries that are experimental and try out different ideas. Once the community plays around with the various libraries one might end up winning, then after we've had enough use and people are happy we can merge the library with qwik itself as the defacto conditional library.

This way we have something that works today and people can play around with various APIs without having the pressure of choosing the "official" one everyone will be forced to use going forward.

Hopefully, in the meantime the qwik team can either implement these compiler based approaches, or help document how we can do this ourselves. This way we the community will also learn more about the internals of qwik and can start help contributing more and driving development of features which the core team might not view as high priority but would still be nice to have.

jdgamble555 commented 1 year ago

In any case, here is my code for what @mhevery seems to agree to:

controls.tsx

import type { JSXNode } from "@builder.io/qwik";

// If Component

export type IF = (props: {
    condition: boolean;
    else?: () => JSXNode;
    children: () => JSXNode;
}) => JSXNode;

export const If: IF = (props) => <>{props.condition ? props.children() : props.else ? props.else() : null}</>;

// For Component

export type FOR = <T, U extends JSXNode>(props: {
    each: T[] | undefined;
    fallback?: () => JSXNode;
    children: (item: T, index: number) => U;
}) => JSXNode;

export const For: FOR = (props) => {

    const { each, fallback, children } = props;

    // case where each is undefined, so could be loading
    if (each === undefined && fallback) {
        return fallback();
    }

    // Qwik can only return one `<></>`, so looks weird
    return <>{
        // case where no array each
        (!each || !Array.isArray(each) || each.length === 0) ? null :

            // else loop through each item
            each.map((item, i) => children(item, i))
    }</>;
};

// Switch Component

export type CASE = (props: {
    where: boolean;
    children: () => JSXNode;
}) => JSXNode;

export const Case: CASE = () => <></>;

export type SWITCH = (props: {
    default?: () => JSXNode;
    children: JSXNode[];
}) => JSXNode;

export const Switch: SWITCH = (props) => {
    for (const c of props.children) {
        if (c.props.where) {
            return c.props.children();
        }
    }
    return props.default ? props.default() : null;
};

Any use it just like we talked about:


<!-- If component -->
<If condition={true} else={() => <>Else Statement</>}>
  {() => <>True if statement</>}
</If>
<If condition={false} else={() => <>Else is true here</>}>
  {() => <>false statement</>}
</If>

<!-- For component -->
<For each={times} fallback={() => {
  console.log('does not load')
  return <>Loading for...</>
}}>
  {(item) => <Test key={item.k} />}
</For>

<!-- Switch Comonent -->
<Switch default={() => <>Default case</>}>
  <Case where={false}>
    {() => <>Case is false</>}
  </Case>
  <Case where={false}>
    {() => <>Case is true</>}
  </Case>
</Switch>

It should be noted that in SolidJS, there is a difference between For and Index depending on how it is rendered. I'm not sure how this translates to Qwik, but it may not be applicable.

These cases don't cover errors, but the TS seems to work well. We could also add ErrorBoundary to catch errors in child omponents, or Suspense to show loading states when child components are loading. Again, may not be applicable for Qwik.

If anyone wants to add anything, please do. I'm ending my comments here for now.

Thanks,

J

RaghavBhat02 commented 1 year ago

https://github.com/BuilderIO/qwik/issues/2678#issuecomment-1404515347 why are lite components better?

jdgamble555 commented 1 year ago

@RaghavBhat02 - see the discord thread

mhevery commented 1 year ago

Just FYI. As of right now, we don't want any sort of compiler solution. So either this can be done with userland, or we don't do it.

@jdgamble555 I like your proposal https://github.com/BuilderIO/qwik/issues/2678#issuecomment-1407278172 any chance you could turn it into a PR with good documentation?

jdgamble555 commented 1 year ago

@adam-r-kowalski @keuller - Did you guys want to make any changes or have any suggestions?

J

keuller commented 1 year ago

@jdgamble555 for me all good. Actually I already implemented it on my project and it is working. I agree with @mhevery let's implement it in userland, for now, I don't believe we need a compile solution. If you want I can proceed with the PR on Qwik.

mhevery commented 1 year ago

If you want I can proceed with the PR on Qwik.

Yes please

keuller commented 1 year ago

@mhevery I need a quick guidance from you. I created a two files:

  1. qwik/src/core/render/jsx/contros.jsx
  2. docs/src/routes/docs/components/controls/index.mdx

Is it good for you? or have you another suggestion? Now I'm trying to build locally and test it before opening the PR. Any guidance about it I also appreciate. πŸ˜ƒ

jdgamble555 commented 1 year ago

@keuller still using my version right?

keuller commented 1 year ago

@jdgamble555 almost, I made some little changes on type names, for instance, FOR to ForControl type.

jdgamble555 commented 1 year ago

@keuller - Do you mind posting it here before you submit it since we have been working on this together.

Thanks,

J

keuller commented 1 year ago

Hi @jdgamble555 sorry for the delay, busy days. The code is:

export type ForControl = <T, U extends JSXNode>(props: {
    each: T[] | undefined;
    fallback?: () => JSXNode;
    children: (item: T, index: number) => U;
}) => JSXNode;

export const For: ForControl = (props) => {
    const { each, fallback, children } = props;

    // case where each is undefined, so could be loading
    if (each === undefined && fallback) {
        return fallback();
    }

    return <>{
        (!each || !Array.isArray(each) || each.length === 0) ? 
            null :
            each.map((item, i) => children(item, i))
    }</>;
};

export type IfControl = (props: {
    condition: boolean;
    else?: () => JSXNode;
    children: () => JSXNode;
}) => JSXNode;

export const If: IfControl = (props) => <>{props.condition ? props.children() : props.else ? props.else() : null}</>;
jdgamble555 commented 1 year ago

Oh you literally just renamed it... okay. I have no issue with that. Just wanted to make sure the code wasn't changed.

Thanks,

J

adam-r-kowalski commented 1 year ago

I have started to use this in my project and will give feedback as I find things out! I will say with the If it's really biased towards the true branch and the else branch seems a bit more hidden.