Adds test for server side rendering

[lukejacksonn]

This is an extension of #7 where most of the work was done by @bebraw 🙇 some things that I took the liberty of changing from that branch included:

• Reverting the change to .mjs extensions and instead using { "type": "module" } in the package json.
• Removes preact from the SSR test and instead just check that otion did its thing properly
• Removing the unused babelrc file
• Gave the tests a package json with otion as a dependency (as the test requires otion/server)

What still remains is:

• Updated vendored version of otion to latest
• Server side rendering test
• Instructions on how to server side render in the readme
• Export of otions setup and hydrate functions

This seems to work pretty nicely for me.. I wonder, is this sufficient for general SSR now?

[bebraw]

This seems to work pretty nicely for me.. I wonder, is this sufficient for general SSR now?

That's a good question. I've worked against my forked version and apart from the issues I opened (separate from SSR), it simply works. 😎

[lukejacksonn]

Awesome! Thanks for the review. I feel this should also just work™️ but it would be great if you could confirm. I will address your other comments now.

[bebraw]

Awesome! Thanks for the review. I feel this should also just work™️ but it would be great if you could confirm. I will address your other comments now.

There's one thing you could add now that I think of it. See https://github.com/bebraw/oceanwind/blob/personal/util/otion.mjs#L71 . That's a fix against Deno for Otion. Otherwise SSR won't work through Deno.

[bebraw]

The issue is that Deno exposes window and I recall their original check used the window object to tell if it's run in a browser or not. That needs an extra check in Deno environment. It should also avoid the process.env problem if done right.

[lukejacksonn]

Ok I have copied over the contents of the file you linked to into this branch. When I tried running it in the browser I got the process is undefined error. So I updated the checks to look like this:

const isDeno = typeof window !== 'undefined' && window.Deno;
const isBrowser = isDeno ? false : typeof window !== 'undefined';
const isDev = isDeno || isBrowser ? false : process.env.NODE_ENV !== 'production';

Note the new isDeno || isBrowser check. Does that make sense to you?

[bebraw]

@lukejacksonn Yeah, that isDeno || isBrowser bit makes sense.

The three lines is starting to feel like some utility to test and extract but there's no need to do that now.

We should try to get this code upstream at some point. 😄

[lukejacksonn]

Yeh I agree. Will have to ask @kripod to take a look and see what he thinks!

[lukejacksonn]

I think this is good to go now. Might merge then publish!

[lukejacksonn]

Ok, got there in the end! Thanks for all your help @bebraw and @jovidecroock 🙇

I will take a look at the other issues now, starting with #10 so that you can start linking from here rather than a fork.

[kripod]

Great job, looks good to me! Thank you for asking me to take a look 😊

[bebraw]

Awesome, thanks a lot. 👍