Skip to content

JSX Tokenizer: add tokens which can be parsed using children #399

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

Open
thetarnav opened this issue Apr 13, 2023 · 9 comments
Open

JSX Tokenizer: add tokens which can be parsed using children #399

thetarnav opened this issue Apr 13, 2023 · 9 comments
Labels
enhancement New feature or request

Comments

@thetarnav
Copy link
Member

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 thetarnav added the enhancement New feature or request label Apr 13, 2023
@bigmistqke
Copy link
Contributor

+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
Copy link
Contributor

bigmistqke commented Jul 3, 2023

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.

@thetarnav
Copy link
Member Author

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.

@bigmistqke
Copy link
Contributor

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?

@otonashixav
Copy link
Contributor

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.

@bigmistqke
Copy link
Contributor

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.

@otonashixav
Copy link
Contributor

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.

@bigmistqke
Copy link
Contributor

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.

@N0tExisting
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants