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

Add optional second parameter to useCreateCtx with custom ctx creation function #1012

Open
artalar opened this issue Jan 20, 2025 · 6 comments
Labels
good first issue Good for newcomers

Comments

@artalar
Copy link
Owner

artalar commented Jan 20, 2025

Discussed in #994

Originally posted by MOZGIII December 30, 2024
I've been doing some development with reatom, and figured we need a way to create a test ctx on a per-page basis.
We have some dev-routes with pages that need mocking - sort of like storybooks but at the same app code as the rest of the app.

import { Fn } from "@reatom/framework";
import { createTestCtx, TestCtx } from "@reatom/testing";
import { useRef } from "react";

export const useCreateTestCtx = (extension?: Fn<[TestCtx]>) => {
  const ctxRef = useRef<null | TestCtx>(null);
  if (!ctxRef.current) {
    ctxRef.current = createTestCtx();
    extension?.(ctxRef.current);
  }
  return ctxRef.current;
};

It would also be fine to have a just more generic way to create and destroy the contexts, so that useCreateCtx doesn't invoke the the createCtx itself but rather allows the user to do so.

import { Ctx, Fn } from "@reatom/framework";
import { useRef } from "react";

export const useCreateCustomCtx = <T extends Ctx>(create: Fn<[], T>) => {
  const ctxRef = useRef<null | T>(null);
  if (!ctxRef.current) {
    ctxRef.current = create();
  }
  return ctxRef.current;
};

The second variant should work better as it does not have direct dependency on both react and a @reatom/testing - which would make it quite puzzling where to place this code.

@artalar artalar added the good first issue Good for newcomers label Jan 20, 2025
@MOZGIII
Copy link

MOZGIII commented Jan 21, 2025

So, useCreateCustomCtx at @reatom/npm-react?

@artalar
Copy link
Owner Author

artalar commented Jan 21, 2025

@MOZGIII there is no need for additional API, base hook would work fine with new parameter. I think, it should be like this:

export const useCreateCtx = <T extends typeof createCtx = typeof createCtx, C extends Ctx = ReturnType<T>>(
  extension?: Fn<[C]>,
  createFn: T = createCtx as T,
): C => {
  const ctxRef = React.useRef(null as null | C)
  if (!ctxRef.current) {
    ctxRef.current = createFn() as C
    extension?.(ctxRef.current)
  }
  return ctxRef.current
}

@MOZGIII
Copy link

MOZGIII commented Jan 21, 2025

Too much generics imo, I'd say we need either T or C generics, not both. Or is there a good reason to have them?

@artalar
Copy link
Owner Author

artalar commented Jan 22, 2025

@MOZGIII it is required for better type safety, but you can try another implementation

@MOZGIII
Copy link

MOZGIII commented Jan 22, 2025

type CreateCtxFn = <C extends Ctx>() => C;

export const useCreateCtx = <T extends CreateCtxFn = typeof createCtx>(
  extension?: Fn<[ReturnType<T>]>,
  createFn: T = createCtx as T,
): C => {
  const ctxRef = React.useRef<null | ReturnType<T>>(null)
  if (!ctxRef.current) {
    ctxRef.current = createFn()
    extension?.(ctxRef.current)
  }
  return ctxRef.current
}

Ok, how about this? Might need to test it, just wrote it here... But the idea is similar, yet different. Here we disallow separating the C type from T by forcing a generic type. It should also allow us to eliminate the as type pinnings and allow TypeScript to derive the code.

The only thing I'm still worried about is this createFn: T = createCtx as T - it is a soundness hole. Although thus is a niche thing, and will likely be fine, I still don't like it. We could probably define this as an overload instead of an optional parameter for better safety if we want this kind of behavior.

@MOZGIII
Copy link

MOZGIII commented Jan 22, 2025

Also, why have this signature in the first place, if we could just do this:

type CreateCtxFn = <C extends Ctx>() => C;

export const useCreateCtx = <T extends CreateCtxFn = typeof createCtx>(
  createFn?: T = createCtx as T,
): C => {
  const ctxRef = React.useRef<null | ReturnType<T>>(null)
  if (!ctxRef.current) {
    ctxRef.current = createFn()
  }
  return ctxRef.current
}

The extension seems kind of redundant.

Plus, it looks like we could just do const ctx = useMemo(createCtx, []); with the same effect.
In our case that createCtx would be our own fn that allows us using test ctx and mocks in development if a certain env var is set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants