Skip to content
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

Confusion around createContext in uland #14

Closed
jaydenseric opened this issue Jun 24, 2021 · 18 comments
Closed

Confusion around createContext in uland #14

jaydenseric opened this issue Jun 24, 2021 · 18 comments
Labels
invalid This doesn't seem right

Comments

@jaydenseric
Copy link

jaydenseric commented Jun 24, 2021

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 WebReflection changed the title Dangerously leaky context values Confusion around createContext in uland Jun 24, 2021
@WebReflection
Copy link
Owner

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
Copy link
Owner

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 WebReflection added the invalid This doesn't seem right label Jun 24, 2021
@WebReflection
Copy link
Owner

@jaydenseric
Copy link
Author

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
Copy link
Owner

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
Copy link
Owner

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

@jaydenseric
Copy link
Author

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Author

jaydenseric commented Jun 24, 2021

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

jaydenseric commented Jun 25, 2021

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
Copy link
Owner

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

Repository owner locked and limited conversation to collaborators Jun 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

2 participants