solidjs / solid-meta

Write meta tags to the document head
127 stars 16 forks source link

[Bug?]: Cannot access context when read in Meta content prop. #41

Open thetarnav opened 9 months ago

thetarnav commented 9 months ago

If you try to read context inside content prop of Meta component provided by start, the context will not be available, which is not expected as it's used directly under the provider.

<Ctx.Provider value={{ text: 'you should see this' }}>
  <Html lang="en">
    <Head>
      <Meta
        name="description"
        content={useContext(Ctx).text} // ctx read here is not available
      />
    </Head>
    <Body>...</Body>
  </Html>
</Ctx.Provider>

https://stackblitz.com/edit/solid-start-meta-context-issue?file=src%2Froot.tsx

I'm interested in working on a fix if this is something that's considered a bug.

pucho commented 9 months ago

Just to add my 2c, my experience is regarding Context on React.

That said, in your example you don't define a default state for your context e.g:

const Ctx = solid.createContext<{ text: string }>({text: 'default ctx'});

With that change the meta tag ends up with it's description being "default ctx", if I had to take a stab at it, at the moment you try to render the root component, the only available value for your ctx is the one on the initial definition.

If you were to move all the Head related stuff to a new component like so:

import * as solid from 'solid-js';
import { Head, Meta, Title } from 'solid-start';
import { Ctx } from './root';

export default function Header() {
  const value = solid.useContext(Ctx);
  return (
    <Head>
      <Title>SolidStart - Bare</Title>
      <Meta charset="utf-8" />
      <Meta name="viewport" content="width=device-width, initial-scale=1" />
      <Meta name="description" content={value?.text} />
    </Head>
  );
}

You'll be able to deal with the context in a better way, your approach feels more like an anti-pattern than a bug.

thetarnav commented 9 months ago

I agree that reading context anywhere but in component body is usually a bad practice. But I have a good reason for this, I'm working on a i18n library that will access context every time you call m.greeting({ name: "John" }) etc, which will be used mostly in JSX, also in <head>. (It needs to read from ctx to avoid state leaking on the server)

Making it into a separate component solves the problem of course, so is inlining one: https://github.com/thetarnav/paraglide-solidstart-hackernews/blob/main/src/root.tsx#L25

I'm considering this a bug because reading ctx in any other prop will work fine:

<Ctx.Provider value={...}>
    <div class={useContext(Ctx)} />
</Ctx.Provider>

thats because props are being read lazily in a computation, which will have access to the context when executed. Meta should be working the same way, since it is a core start component, that's meant to match the native <meta> tag. Also resolving children works differently in solid compared to react. That code would be invalid in React, while in Solid it depends on how well the component is implemented. That same template when ported to react would look more like this:

<Ctx.Provider value={{ text: 'you should see this' }}>
  {() => <Html lang="en">
    {() => <>
        <Head>
        {() => <Meta name="description" content={() => useContext(Ctx).text} />}
        </Head>
        <Body>...</Body>
    </>}
  </Html>}
</Ctx.Provider>

because props are compiled into getters and read lazily, so one could argue that there are many additional components between the provider and the ctx read.

ryansolid commented 8 months ago

Yeah.. it's because of the lazy access of meta tags to insert them in the head. I see its in the head there but we don't really need to render them until we collect them all. I guess this is a solid meta issue. I will move there.

aguilera51284 commented 3 months ago

any solutions for this?