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

fix(ssr): problem with document not existing on server side #2596

Merged
merged 3 commits into from
Jan 20, 2024

Conversation

stephenjason89
Copy link
Contributor

Description

Fixed the problem where document doesn't exist on server side

Checklist

  • Created tests which fail without the change (if possible)
  • All tests passing (yarn test)
  • Extended the README / documentation, if necessary

@stephenjason89 stephenjason89 changed the title fix(ssr): problem with document doesnt exist on server side fix(ssr): problem with document not existing on server side Jan 12, 2024
src/utils.ts Outdated
@@ -56,7 +56,7 @@ export class Utils {
/** convert a potential selector into actual list of html elements. optional root which defaults to document (for shadow dom) */
static getElements(els: GridStackElement, root: HTMLElement | Document = document): HTMLElement[] {
if (typeof els === 'string') {
Copy link
Member

Choose a reason for hiding this comment

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

I would just return if root is null instead of all these checks...

Copy link
Contributor Author

@stephenjason89 stephenjason89 Jan 13, 2024

Choose a reason for hiding this comment

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

Do you mean return undefined since root is undefined on server side?

I can actually just revert this file since init and initAll will not call this anyway when document is undefined.

I just changed this just in case you'd like to use these functions outside init and initAll without worry if it's being called in ssr.

Would you want me to revert?

Copy link
Member

Choose a reason for hiding this comment

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

yes if not needed, or better fix as I suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, i have reverted the file.

@@ -94,6 +94,9 @@ export class GridStack {
* let grid = document.querySelector('.grid-stack').gridstack;
*/
public static init(options: GridStackOptions = {}, elOrString: GridStackElement = '.grid-stack'): GridStack {
if(typeof document === 'undefined') {
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 how returning null and doing nothing on the server side is going to help anything - not only will the dom not be created with correct html, but the caller will likely crash refering the grid.

Copy link
Contributor Author

@stephenjason89 stephenjason89 Jan 13, 2024

Choose a reason for hiding this comment

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

We want it to not do anything in the server side, like accessing document because this is undefined there. Since DOM is not present there, we cant manipulate the DOM during SSR.

not only will the dom not be created

Don't worry because init or initAll would be called again in client side and everything will be rendered properly.

@fredericrous
Copy link
Contributor

fredericrous commented Jan 13, 2024

I tested this PR, the warnings indeed are removed thanks. everything looks ok. and remix doesn't crash anymore \o/ . I think this PR is a first step toward ssr compatibility

Of course in an ideal world, supporting srr would mean that gridstack serves a prebuilt grid, created server side.

@stephenjason89
Copy link
Contributor Author

@fredericrous thank you for testing it out. Indeed, a prebuilt grid would be nice. I hope to see it soon as well 😊

@adumesny
Copy link
Member

| Of course in an ideal world, supporting srr would mean that gridstack serves a prebuilt grid, created server side

my point exactly - this feels like a bandaid. the all point of SSR is that all the HTML (and hence the doms attr, style and widget content which can be created when loading a grid definition (eg angular components) are rendered and sent to the user before all JS code is sent over.

And why would init() be calledin the server (no-op) AND client side (actually do something) ? everything runs twice then and need to handle already done stuff (which GS does I beleive).

@fredericrous
Copy link
Contributor

If I'm understanding your message correctly, you are worried about getting an extra rerender on hydration?

what I understand of ssr:

  • it reduces initial loading time
  • it gives old school search engine's scrappers data to index

I doubt we would see any noticeable perf regression with calling init on server and on client, quite the contrary, the page will load faster and user won't notice the refresh. Plus we have to take into account some of these frameworks that offer ssr, can also build a spa. Calling init both and server and client seems the simplest

regarding the Content of the widgets on server side, on React and Vue we can set it with the function renderToString

@adumesny
Copy link
Member

no my worry is that init/load can do a lot more work than just setting dom attributes. in dynamic cases it causes the entire widget and content to be created dynamically. so you would miss the entire page html rendering ahead of time, which is the all point of SSR. eg https://github.com/gridstack/gridstack.js/blob/master/angular/projects/demo/src/app/app.component.ts#L41

@fredericrous
Copy link
Contributor

fredericrous commented Jan 17, 2024

I'm not sure I understand your point. Discussing this on a forum might not be the best approach.

My perspective: we determine what to serve based on the presence of window.document. For server-side rendering (SSR), we can't use document.querySelector or addEventListener.

When window.document is available, we attach event listeners and use our preferred framework to manage the content within widgets. For example, I use React to replace the content of a widget by identifying its ID, rather than using the widget's "content" method. On the other hand, in SSR where window.document is absent, we could use the renderToString function from our respective frameworks (in my case, I'd use the "content" method to inject html from renderToString).

GridStack heavily relies on querySelectorAll, for instance, to identify all subgrids. In SSR, this could be substituted with a recursive function that iterates over the GridStack instance, checking for gridstackNode.subGrid and recursively calling itself. There could be more efficient solutions to reduce time complexity

Regarding helping on SSR feature, at this time I can only offer to help write Specifications and do Reviews. I need to get my MVP working, I have no revenue yet. And SSR will be a need if my project works but at this time it is a nice to have
In the mid-time, Stephen's PR prevents frameworks like Remix to crash. It's not a hard crash but it does spam the console heavily. I would merge it once the fix you asked are implemented.

@stephenjason89
Copy link
Contributor Author

my point exactly - this feels like a bandaid. the all point of SSR is that all the HTML (and hence the doms attr, style and widget content which can be created when loading a grid definition (eg angular components) are rendered and sent to the user before all JS code is sent over.

Indeed, this is precisely the case. The primary aim is to circumvent the errors that occur in SSR when Gridstack attempts to access the DOM. Although I eagerly anticipate a genuine SSR implementation where a grid can be initially set up, I currently lack the time to undertake this task.

I'm looking forward to the prompt merging of this PR, as it will help us avoid SSR-related errors. However, having complete support for SSR would be highly beneficial and is greatly desired.

@adumesny adumesny merged commit 88c99d3 into gridstack:master Jan 20, 2024
adumesny added a commit to adumesny/gridstack.js that referenced this pull request Jan 20, 2024
@adumesny adumesny mentioned this pull request Jan 20, 2024
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