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

feat(server): added hydrate functionality #1

Merged
merged 10 commits into from Jan 17, 2021
Merged

feat(server): added hydrate functionality #1

merged 10 commits into from Jan 17, 2021

Conversation

jacob-ebey
Copy link
Contributor

@jacob-ebey jacob-ebey commented Jan 16, 2021

Added hydrate functionality to work in conjunction with https://www.npmjs.com/package/forgo-render-to-string

Updated package.json to work in multiple environments (windows)

@jacob-ebey
Copy link
Contributor Author

On second thought, this should be it's own package. I don't want it to blow up the size of the client bundle.

@jeswin
Copy link
Member

jeswin commented Jan 16, 2021

Hi Jacob, thanks for your inputs. It's a great feature to add, but like you said - being minimal (and having zero dependencies) is absolutely critical to forgo. So I'd say this probably needs a little more discussion. We could use this thread - let me spend some time on it as well.

But great experiment. Looking forward to integrating it at some point.

@jacob-ebey
Copy link
Contributor Author

jacob-ebey commented Jan 16, 2021

@jeswin I've removed render-to-string functionality from the core forgo package in favor of a separate package. This PR now contains just the hydration functionality required for the client.

The render-to-string package can be found here:
source: https://github.com/jacob-ebey/forgo-render-to-string
npm: https://www.npmjs.com/package/forgo-render-to-string

While an example utilizing hydrate can be found here:
https://github.com/jacob-ebey/forgo-ssr-example

src/index.ts Outdated Show resolved Hide resolved
@jacob-ebey jacob-ebey changed the title feat(server): added hydrate and render to string functionality feat(server): added hydrate functionality Jan 16, 2021
updated package.json scripts to work on windows
@jacob-ebey
Copy link
Contributor Author

@jeswin I've added tests and updated to default to selecting first child instead of a querySelector.

I'd be good with cutting a beta of this. It's working fine for me at this point, grated I don't have a super complex use-case, just what can be found here: https://github.com/jacob-ebey/forgo-ssr-example and the button click test added in this PR.

@jacob-ebey jacob-ebey mentioned this pull request Jan 17, 2021
@jeswin
Copy link
Member

jeswin commented Jan 17, 2021

@jacob-ebey Thanks for the tests! Let me review this by tomorrow (Monday) and then get it merged.

@jeswin
Copy link
Member

jeswin commented Jan 17, 2021

I did a cursory review of hydrate. Let me split that into two parts

Part 1:

const opts = options || {};
if (parentElement) {
    let root;
    if (typeof opts.root === "function") {
      root = opts.root();
    } else if (typeof opts.root === "object") {
      root = opts.root;
    } else if (typeof opts.root === "string") {
      root = parentElement.querySelector(opts.root);
    } else {
      root = parentElement.firstElementChild;
    }
}

Part 2:

const { node } = render(forgoNode, undefined, [], false);
parentElement.replaceChild(node, root);

Q1: Now, wouldn't it be possible to do Part 1 entirely outside forgo? There doesn't seem to be any forgo related code in those lines.

Q2: Now Part 2 certainly needs to be in the lib. It's very similar to what mount does, but you need 'replaceChild' instead of 'appendChild'. How about we export something like render(forgoNode: ForgoNode, forceFullRender: bool), which simply returns the newly created node - and the caller can use it to do a replaceChild?

@jacob-ebey
Copy link
Contributor Author

jacob-ebey commented Jan 17, 2021

@jeswin That works for me. I've made the changes. I'd actually like to see the full result of the internal render returned, not just the node. This will enable things like error boundaries.

@jeswin jeswin merged commit 344448b into forgojs:main Jan 17, 2021
@jeswin
Copy link
Member

jeswin commented Jan 17, 2021

Thanks for the awesome PR - your analysis of the existing code base and the work on top of it has been truly impressive!

@jeswin
Copy link
Member

jeswin commented Jan 17, 2021

I think it might be useful to have a tiny paragraph in the README discussing SSR - and linking to the forgo-render-to-string library you created (if it's ready for use). What do you think?

Add: I edited your README.md change to remove the mention of SSR in the render() API section, since it didn't really explain how to actually achieve SSR (and hence might be confusing). Hence proposing that we actually add an SSR specific section.

@jacob-ebey
Copy link
Contributor Author

Let's hold off until we go over suspense/error handling in the render cycle. I'd like to enable async suspense based rendering on the server if the error proposal is accepted.

@jacob-ebey jacob-ebey deleted the hydrate branch January 17, 2021 10:31
@jeswin
Copy link
Member

jeswin commented Jan 17, 2021

Additionally, I've removed the fullRender parameter from render(). The parameter has no effect, since without a node being passed in, it will do a full render anyway. Partial render relies on pulling previous state from the DOM tree, which in this case doesn't exist.

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.

None yet

2 participants