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

[WIP] Make router operations injectable #288

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tannerlinsley
Copy link

  • Makes router operations injectable
  • Makes router context generic
  • Still adheres to Remix's sessionStorage and session API's

@tannerlinsley tannerlinsley marked this pull request as draft July 17, 2024 19:03
@tannerlinsley tannerlinsley changed the title Make router operations injectable [WIP] Make router operations injectable Jul 17, 2024
@sergiodxa sergiodxa self-requested a review July 17, 2024 19:25
@@ -74,6 +75,8 @@
"typescript": "^5.1.6"
},
"dependencies": {
"@tanstack/react-router": "^1.45.4",
"@tanstack/start": "^1.45.4",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't end up here. It would likely end up in a tanstack-auth package along with the TanStackAuthenticator class.

sessionKey?: AuthenticateOptions<TContext>["sessionKey"];
sessionErrorKey?: AuthenticateOptions<TContext>["sessionErrorKey"];
sessionStrategyKey?: AuthenticateOptions<TContext>["sessionStrategyKey"];
throwOnError?: AuthenticateOptions<TContext>["throwOnError"];
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds the sessionStorage to the options to bring things down to a single required options bag. This is breaking (but then again, much of this is), but has proven to be a good change in our TanStack libraries.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a non-breaking change if the current API is still implemented by the RemixAuthenticator

redirect,
});
}
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also imagine that this would need to stay in the "remix-auth" package while most everything else would need to go into some core package.

Session,
SessionStorage,
} from "@remix-run/server-runtime";
import { isSession, Session, SessionStorage } from "@remix-run/server-runtime";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still using a Remix package, would it be ok for TSS to depend on this? This doesn't really depend on anything from Remix, maybe it could convince the team to move them to a separate agnostic package?

Another option could be to make Authenticator use another interface like's unstorage and provider wrappers of SessionStorage for unstorage in the Remix package and another in the TanStack package.

Comment on lines +19 to +25
export interface AuthenticatorImplOptions {
redirect: (
url: string,
options?: { headers?: HeadersInit }
) => Response | Promise<Response>;
json: <T>(data: T, status?: number) => Response;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to provide this from TanStack? At least in Remix they only wrap new Response(), in that case they could be implemented right here

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

Successfully merging this pull request may close these issues.

2 participants