solid-primitives icon indicating copy to clipboard operation
solid-primitives copied to clipboard

JSX Tokenizer: add tokens which can be parsed using `children`

Open thetarnav opened this issue 2 years ago • 9 comments

Describe The Problem To Be Solved

Currently, the component returned by createToken will return a function element. This way we can display a friendly warning in the console if something tries to render the token, instead of an error from trying to insert a non-JSX.Element type. The downside of that approach is that the token elements cannot be passed to a context provider, transition-group, or anything that tries to resolve the children using the children helper, even if it doesn't necessarily render anything yet. So we lose a bit of functionality, and composition value, for a nicer misuse warning.

Suggest A Solution

I see this being implemented in two ways. Either the createToken component will be returning an object, instead of a function if the fallbackFn param is not provided. Or there could be createToken and createRenderableToken as separate functions.

thetarnav avatar Apr 13 '23 15:04 thetarnav

+1 the whole not-resolving-tokens in jsx-tokenizer is a bit finnicky and brings mental overhead that makes me less want to use it (or advice others to use it). Especially since solid itself perfectly allows for non-DOM to be returned (only typescript is unhappy about that). Typecasting objects to JSX.Element, in combination with the typing that jsx-tokenizer provides would be typesafe enough imo.

bigmistqke avatar Jun 19 '23 20:06 bigmistqke

little experiment with simply casting: https://playground.solidjs.com/anonymous/5110caf3-7129-411b-9da3-7ff423f6d296

const token = (args[0] ? args[0](props) : props) as unknown as TokenElement<T>;
token[$TOKENIZER] = symbol;
return token as unknown as JSX.Element;

then we can only accept objects as return-values, but the abstraction looks a bit cleaner:

const Token = createToken(() => ({ id: 'string' }));

const Parent = (props: {children: JSX.Element)) => {
  const token = props.children;
  token.id // 'string'
  // instead of
  token.data.id // 'string'
})

We get the following warning if a token accidentally gets added to the DOM-tree in dev-mode, so don't lose error-handling completely:

Screenshot 2023-07-03 at 18 43 37

Context becomes a lot easier too:

const childs = <Context.Provider value={...}>{props.children}</Context.Provider>

just works.

This is 💯 the way forward.

bigmistqke avatar Jul 03 '23 16:07 bigmistqke

It's more or less what I want to do but I without adding a property to the token (token[$TOKENIZER] = symbol) But instead use a WeakMap for identifying passed objects.

thetarnav avatar Jul 03 '23 18:07 thetarnav

Yes, WeakMap is a good idea!

We wouldn't really need to resolveTokens anymore in this next version, but I guess there is still a need to filter props.children for tokens, even just for the nice typings, so maybe filterTokens instead of resolveTokens?

bigmistqke avatar Jul 04 '23 09:07 bigmistqke

I haven't really touched this library but hopefully the idea gets across.

I'm not sure where this is at the moment, but I don't believe using an object solves the issues outlined. Instead, it may make errors more difficult to spot. Consider the following:

<Resolver>
  <Context.Provider>
    <Token>
      <Context.Consumer />
    </Token>
  </Context.Provider>
</Resolver>

Using an object, the consumer would fail to read the context since it would be executed outside the provider. Similarly, with TransitionGroup, anything inside the token wouldn't be animated (I can't imagine you passing a token straight to TransitionGroup in the first place though). In both cases, you won't get the (arguably) expected behaviour, but instead of seeing a helpful error you'd get either nothing or "Are you calling this inside the context provider?".

In other words, consider keeping the current implementation the primary one, and adding a createObjectToken instead.

otonashixav avatar Dec 14 '23 10:12 otonashixav

Hi @otonashixav

Not sure if I completely follow. I have been using this pattern with context without any problems. Not too familiar with TransitionGroup.

From my experiments in this playground I was only able to reproduce the context-bug you described by passing the children as a getter to the resolver, and that feels pretty intuitive to me.

You mentioned in discord that adding an unused argument would prevent it from being resolved with children, but what about Show? see playground

Having objects as the containers is just less finnicky then functions imo.

bigmistqke avatar Dec 16 '23 13:12 bigmistqke

GoodToken1 won't update when props.children changes. GoodToken2 is fine, but is dependent on being able to resolve the children immediately, which is not always the case; the resolver may want to execute that step under different circumstances (different roots, different contexts, etc.).

Yes, I mentioned in my playground example that the unused parameter would not work properly with Show. In any case it was an example of what could work, not what to actually use -- I suggested this library because I thought it was the most foolproof.

otonashixav avatar Dec 17 '23 03:12 otonashixav

GoodToken1 won't update when props.children changes.

Yes, I know, It was just to indicate that context worked as expected.

GoodToken2 is fine, but is dependent on being able to resolve the children immediately which is not always the case; the resolver may want to execute that step under different circumstances (different roots, different contexts, etc.).

I don't think I follow here. What would be the usecase? Sounds like behavior that would break how you would expect components to behave regularly. I don't really understand how using functions as data-containers would help here.

  • you can actually do this if you really want to, and it works quite intuitively: https://playground.solidjs.com/anonymous/bf38f394-f56a-4d59-81d4-a541920cece6

bigmistqke avatar Dec 17 '23 09:12 bigmistqke

I think this creates other issues with props spread, see: https://stackblitz.com/edit/solid-jsx-tokenizer-spread?file=src%2Flib%2Findex.tsx

N0tExisting avatar Jun 10 '24 15:06 N0tExisting