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

Small fixes for SSR #4242

Merged
merged 3 commits into from
Jan 8, 2020
Merged

Small fixes for SSR #4242

merged 3 commits into from
Jan 8, 2020

Conversation

CarsonF
Copy link
Contributor

@CarsonF CarsonF commented Jan 3, 2020

It doesn't seem like SSR is supported, and the process is complicated when it involves data fetching. Maybe it will never be supported out of the box, but I'm hoping I can make some changes to make it possible with a custom setup.

These are two small changes just to allow rendering to be successful server side (even though data is incomplete).

  • ConnectedRouter interferes with StaticRouter and can just be ignored server side.
    Add a more explicit note on SSR supasate/connected-react-router#322
  • Title component was assuming document exists. Now it just silently fails, which is the same as not finding the #react-admin-title container.
    What we really need here is a portal implementation that works server side. There's some POCs for this, but they are just that. I'm still thinking about a better way to implement this.

Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Thanks!

@djhi djhi added this to the 3.1.2 milestone Jan 6, 2020
{children}
</ConnectedRouter>
) : (
children
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand: calls to <Route> in react-admin components (e.g. <Resource>) will fail if there is no router. This can only work if developers wrap their entire Admin in a StaticRouter. We should detect that a router is already provided and throw a nice exception if there is none, otherwise the developer experience will be confusing.

Or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... react-router will throw an error if Route is used outside of Router
https://github.com/ReactTraining/react-router/blob/master/packages/react-router/modules/Route.js#L35

I believe this is enough and that it shouldn't be the responsibility of AdminContext to enforce that.
BUT say the word and I'll add a check and a different(?) error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can only work if developers wrap their entire Admin in a StaticRouter.

And yes, all SSR frameworks do this (or require the dev to do it) as it requires tight integration with the server's request.

@@ -13,7 +12,6 @@ import { I18nProvider } from '../types';
interface Props {
locale?: string;
i18nProvider: I18nProvider;
children: ReactElement<any>;
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand that change

Copy link
Contributor Author

@CarsonF CarsonF Jan 6, 2020

Choose a reason for hiding this comment

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

FunctionComponent has children defined as an optional ReactNode

interface FunctionComponent<P = {}> {
    (props: PropsWithChildren<P>, context?: any): ReactElement | null;
    ...
}

type PropsWithChildren<P> = P & { children?: ReactNode };

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/24f1d0c82da2d898acd03fbb3e692eba3c431f82/types/react/index.d.ts#L517-L518
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/24f1d0c82da2d898acd03fbb3e692eba3c431f82/types/react/index.d.ts#L773

By defining children in the TranslationProvider props we are restricting it further from what React accepts. This is incorrect. Context Provider's accept anything really as their children.

The change is also needed because TypeScript complains about TranslationProvider usage in CoreAdminContext because children there is the same loose type instead of the strict ReactElement.

@CarsonF CarsonF requested a review from fzaninotto January 7, 2020 21:23
@fzaninotto fzaninotto merged commit c633422 into marmelab:master Jan 8, 2020
@fzaninotto
Copy link
Member

Thanks!

@CarsonF CarsonF deleted the bugfix/ssr branch January 15, 2020 23:12
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.

3 participants