Confusion around createContext in uland

jaydenseric commented 3 years ago

Currently, the way to create and use context is like this (edit: more correct code here):

import {
  createContext,
  html,
  render,
  useContext,
} from "https://cdn.skypack.dev/uland-ssr@0.4.0";

const FooContext = createContext();

FooContext.provide("abc");

const renderedHtml = render(
  String,
  () =>
    html`<!DOCTYPE html>
<html>
  ${useContext(FooContext)}
</html>
`,
);

console.log(renderedHtml);

$ deno run demo.js
<!DOCTYPE html>
<html>
  abc
</html>

It took me a lot of time to figure this out by reading the source code, because there is zero API documentation for using context.

But, the huge problem with the way context works is that a context value is set in the created context, instead of being scoped to the current render tree via provider components. This is a huge deal! It means if you are server side rendering multiple requests from different users at once, that the context values will be incorrectly shared for the separate renders, and race conditions determine which values are set and read.

Another issue, it it's not clear to me if context values can be overridden in components when rendering children, without affecting the context value for components rendering outside of the children branch. That's something React context does.

I'm building a SSR web app framework that needs to provide the HTTP request and response in context for the server side render, so deep in components anyone can do useContext(RequestContext) for example, to get cookies and things from the request. With my next-server-context package I was able to achieve this for Next.js/React apps.

Is such a thing impossible for uland?

WebReflection commented 3 years ago

Let's start saying that for SSR there is an ad-hoc project called ... 🥁🥁🥁 ... uland-ssr.

Now, let's assume you really mean to use a client-side FE library to render SSR ... here a few points:

the createContext is scoped as much as you want it to be.
it's true that the documentation around useContext here is not superb, if not completely absent, but the context is not "a live component" in uhooks, it's more like a shared ref across components using such context, so that whenever it provides a new value, all components using it will update accordingly.

Back to the first point: if you create a context in the global scope and share it with every connection, you are causing yourself an issue.

Not only the example you shared does that, but you are not using Component at all.

If you don't create components, nothing work here, so, if you need a globally shared information, but you don't want hooks, because these won't work SSR, you either use uhtml-ssr or you gotta create components.

Nothing in uland, uhooks, works, if you don't wrap your callbacks in components, so once you'll provide an example that makes more sense, and it doesn't share globally the same context across all components, I'll try to figure out what is your issue.

So far, I've read a very drama-oriented issue title with an example that makes no sense because no component was used, and the render is just the uhtml one, if you don't use components, and the context is just represented as value.

TL;DR

bad issue title
bad example
bad usage of this module in the example
no reproducible issue, as the example is not how you use this module

So please help me out helping you, or fix any bug, in a better way, thank you 👋

WebReflection commented 3 years ago

P.S. in case it's not clear, you cannot use any utility outside components ... and this is the premises of hooks. You use hooks outside components? you're doing it wrong. This is documented everywhere around my hooks based projects.

You can start here though, in case you are confused about how hooks work: https://webreflection.medium.com/demystifying-hooks-f55ad885609f

The long story short: no Component(callback), no hooks.

WebReflection commented 3 years ago

Some example around context, in case it helps:

jaydenseric commented 3 years ago

Read the point I'm making, instead of dismissing this issue. In my project code, I'm calling useContext inside a component, you're right that I've overly simplified the code in the issue for you, but here is the fixed version (my concerns still stand):

import {
  Component,
  createContext,
  html,
  render,
  useContext,
} from "https://cdn.skypack.dev/uland-ssr@0.4.0";

const FooContext = createContext();

const Page = Component(() => {
  const value = useContext(FooContext);
  return html`<div>${value}</div>`;
});

FooContext.provide("abc");

const renderedHtml = render(
  String,
  () =>
    html`<!DOCTYPE html>
<html>
  ${Page()}
</html>
`,
);

console.log(renderedHtml);

if you create a context in the global scope and share it with every connection, you are causing yourself an issue.

That's literally the point I'm making. Context values are in the global scope; there are no ways to scope a context value per render. I want to be able to do this:

import { Component, html } from "https://cdn.skypack.dev/uland-ssr@0.4.0";
import RequestContext from "./RequestContext.js";
import ResponseContext from "./ResponseContext.js";

const App = Component(({ request, response, children }) => {
  RequestContext.provide(request);
  ResponseContext.provide(response);

  return html`<body>
  ${children}
</body>`;
});

export default App;

And then be able to render the app, passing in the context values to be provided just for that render.

WebReflection commented 3 years ago

that ain't gonna work, because provide updates components, so it's more like a call stack overflow in uland, and context in uland is not the same as in React, and probably never will, unless I find the time to think and refactor it.

Out of curiosity though: this use case looks better with a useRef instead, unless your children import the context too from those files, but then again, you are updating all children with the new request, response, and if those children has already been invoked, they'll get the previous context value, not the current one, as that .provide call is too late.

That being said:

Read the point I'm making, instead of dismissing this issue

You are giving for granted you are writing perfect code, but all I can work on, is reproducible examples that reason with a possible bug. So far everything looks by design to me, which doesn't mean the uhooks context design is great, but it does mean I would expect, from your re-written example, exactly what you see.

Could you write a minimal example that works in terms of expectations and doesn't ask me to create files I've no idea what's inside?

WebReflection commented 3 years ago

Actually, this bug, or enhancement request, doesn't really belong here ... this is where createContext exists, so we better move over there.

jaydenseric commented 3 years ago

Could you write a minimal example that works in terms of expectations

Impossible, because the current API doesn't allow context to be used the way we need it to.

To explain the use case really high level, I'm creating an alternative to Next.js for use in Deno, using your template string based rendering solutions to avoid any build steps. A framework for SSR apps.

Something Next.js has, is a useRouter hook:

https://nextjs.org/docs/api-reference/next/router#userouter

Its implementation basically just gets the router from context.

During SSR, it's really important that the router object put into the router context is scoped only for the current request/render.

The useRouter hook (or useContext(RouterContext)) and other hooks like useRequest and useResponse need to be able to be imported from my to-be-published framework, into user's project code and even other packages that might be published separately. For example, separately published component UI libraries.

We can't build realistic web apps without a proper context solution, so I really hope there is a way to get this going using uland or I'll be forced to abandon it for some other rendering solution.

From one indie package author to another; it's a tragedy your lightweight template string based rendering solutions don't have wider adoption. I want to help you with that; if a 10 year JS veteran and package author like me can't adopt them for basic web apps, then what hope does the public have.

WebReflection commented 3 years ago

From one indie package author to another, it's a tragedy nobody is contributing to my libraries but many have high expectations out of the box about every single aspect of my libraries.

I'll try to make the context right, as I understand the use case, and I know currently the context is a bit awkward, but the right library for SSR is still uland-ssr, imho, so I never bothered with those components to react to a context at all, as providing new context requires an update for all components, and that's usually not super friendly, in terms of SSR.

Anyway, I'll close this, and open an issue where it belongs.

WebReflection commented 3 years ago

by any chance I can have a children idea? what are those? any child example might help me figuring out what you're after in here, as the useContext, if already invoked in those children, needs some extra logic (I think).

jaydenseric commented 3 years ago

For the app, children is intended to be whatever the page component (dynamically imported according to the current route pathname) renders.

I ran into the problem you mentioned, about the child component rendering before the context value is set in the parent. The way I worked around it was something like this:

import { Component, html } from "https://cdn.skypack.dev/uland-ssr@0.4.0";
import RequestContext from "./RequestContext.js";
import ResponseContext from "./ResponseContext.js";

const App = Component(({ request, response, PageComponent, ...pageProps }) => {
  RequestContext.provide(request);
  ResponseContext.provide(response);

  return html`<body>
  ${PageComponent(...pageProps)}
</body>`;
});

export default App;

This way the child page component renders after the context values have registered in the app component.

WebReflection commented 3 years ago

Yeah, use context is synchronous, and so are components callback, so when they get the context value, is the previous one, and provide would re-invoke those components, but they returned value would be already late.

Any chance you can run

RequestContext.provide(request);
ResponseContext.provide(response);

before the main App component is invoked?

jaydenseric commented 3 years ago

Yes, but there needs to be a way that the value of that context only applies within the render tree of the app. At the time I assumed that would necessitate the value being passed in as a prop like how React context providers are populated in the render tree.

jaydenseric commented 3 years ago

There are client side use cases for isolated context too; for example, what if you had a ColorScheme context, and you wanted to render different parts of the same screen in light and dark mode at the same time. People have video components, that you can drop certain control components into the children, and they will automatically control the parent video via context. If you have 2 videos on the page, you don't want the context for the second to corrupt the context for the first and cause the second video's controls to control the first video (or the other way around).

WebReflection commented 3 years ago

I am actually having hard time reproducing your issue. The component, once invoked, does not invoke the underlying callback, it just returns a Hook instance. These instances are resolved only when the component is rendered.

see this live example

import {
  Component, render, html,
  useContext, createContext
} from '//unpkg.com/uland?module';

const shared = createContext(123);

const Inner = Component(() => {
  const value = useContext(shared);
  return html`<p>${value}</p>`;
});

const Outer = Component((children) => {
  shared.provide(456);
  return html`<div>${children}</div>`;
});

render(document.body, html`
  <div>${Outer([Inner(), Inner(), Inner()])}</div>
`);

I start believing you pass children that were rendered already, or maybe I am not understanding what is your issue?

Could you use this basic example to demonstrate what's breaking or what's unexpected?

jaydenseric commented 3 years ago

I am actually having hard time reproducing your issue.

Here is a reproduction, of just the timing problem which is what I think you're referring to:

import {
  Component,
  createContext,
  html,
  render,
  useContext,
} from "https://cdn.skypack.dev/uland-ssr@0.4.0";

const FooContext = createContext();

const Inner = Component(() => {
  const value = useContext(FooContext);
  return html`<div>${value}</div>`;
});

const Outer = Component(({ fooValue, children }) => {
  FooContext.provide(fooValue);
  return html`<div>${children}</div>`;
});

const renderedHtml = render(
  String,
  () =>
    html`<!DOCTYPE html>
<html>
  ${Outer({ fooValue: "123", children: Inner() })}
</html>
`,
);

console.log(renderedHtml);

$ deno run demo.js
<!DOCTYPE html>
<html>
  <div><div></div></div>
</html>

Note that the context value 123 is not in the rendered HTML.

WebReflection commented 3 years ago

So ... It was about uland-ssr after all ... why did you open an issue in here? Did I mess up the package.json there?

jaydenseric commented 3 years ago

Regarding race conditions due to the API design, here is an example of what happens if there is any sort of time delay while rendering in parallel:

import {
  Component,
  createContext,
  html,
  render,
  useContext,
} from "https://cdn.skypack.dev/uland-ssr@0.4.0";

const FooContext = createContext();

const FooComponent = Component(() => {
  const value = useContext(FooContext);
  return html`<div>${value}</div>`;
});

async function doRender(fooValue) {
  FooContext.provide(fooValue);

  // Realistically, a delay would happen inside rendering of complex components.
  await new Promise((resolve) => setTimeout(resolve, 1));

  const renderedHtml = render(
    String,
    () =>
      html`<!DOCTYPE html>
  <html>
    ${FooComponent()}
  </html>
  `,
  );

  console.log(renderedHtml);
}

doRender("123");
doRender("456");

$ deno run demo.js
<!DOCTYPE html>
  <html>
    <div>456</div>
  </html>
<!DOCTYPE html>
  <html>
    <div>456</div>
  </html>

WebReflection commented 3 years ago

The async example is not so interesting, uland-ssr/async is what you should use there. I’m closing this issue and follow up comments as this is the wrong repository for uland-ssr

WebReflection / uland

Confusion around createContext in uland #14