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

Deno compatibility #8

Closed
katywings opened this issue May 18, 2020 · 20 comments
Closed

Deno compatibility #8

katywings opened this issue May 18, 2020 · 20 comments
Labels
enhancement New feature or request

Comments

@katywings
Copy link

katywings commented May 18, 2020

Motivation

I am trying to make Otion compatible with Deno. As a poc I created this little transform script: https://github.com/lufrai/deno-otion/blob/master/update.ts

Details

It already looks like Otion is mostly working in Deno with the changes. Is this something you would be interested in to integrate into your Otion builds?

One thing thats still a mystery is Otions server side rendering: in my Deno tries it only works for the first request, even though I run setup with a new VirtualInjector like here on each request. The style tag is just empty on all requests after the first one 😅🙈. Since i only tried it out with Deno yet, I am unsure if this might be in node as well.

Edit: From looking at the Otion code I think the problem mentioned at the top with the style tag being empty after the first render actually also exists in node:
The injector only gets new css here: https://github.com/kripod/otion/blob/master/packages/otion/src/createInstance.ts#L183. But the setup call never deletes the insertedIdentNames from previous ssr.

@katywings katywings added the enhancement New feature or request label May 18, 2020
@kripod
Copy link
Owner

kripod commented May 18, 2020

Whoa, thank you for the detailed suggestion! Sure, Deno should be supported out of the box. The windowdocument change is an easy one to start with. After that, I think Rollup should be configured to create separate dev and prod builds, so import maps could resolve to either of them (defaulting to the development production build). Please let me know if there is a better convention I'm not being aware about.

I would appreciate if you could set up a minimal reproduction case for testing SSR, even with Node. Thank you for all the efforts put into this!

@katywings
Copy link
Author

Hey @kripod
Thank you for your quick response (: i see that you already committed the window -> document change, awesome!

Regarding process.env.NODE_ENV: I actually couldnt find any clear Deno related best practice but the same topic is discussed in graphql/graphql-js#2409. Well today I will not have time to read through this though 😂.

I probably can create you a test script and proposed fix regarding ssr at wednesday/thursday ish.

@kripod
Copy link
Owner

kripod commented May 18, 2020

Wonderful, thank you very much!

@katywings
Copy link
Author

katywings commented May 19, 2020

@kripod Earlier than expected: here you go :), a little codesandbox with some tests and a proposed fix in otion setup (look for katyFix :D)
https://codesandbox.io/s/modest-varahamihira-9l4gr?file=/src/index.js

P.S. if you make changes in the code on codesandbox you should reload the inline browser manually because codesandbox does some hmr and keeps the old insertedIdentNames in memory as well (when my fix is disabled ;))

@kripod kripod closed this as completed in e54209a May 19, 2020
@kripod
Copy link
Owner

kripod commented May 19, 2020

Thank you for the proposed fix! Meanwhile, I've released v0.2 which should offer Deno support out of the box. Please use import maps as shown:

/* import_map.json */

{
   "imports": {
      "otion/dev": "https://unpkg.com/otion@0.2.0/dist-deno/bundle.dev.mjs",
      "otion": "https://unpkg.com/otion@0.2.0/dist-deno/bundle.prod.min.mjs"
   }
}

@katywings
Copy link
Author

katywings commented May 19, 2020

@kripod But did you see the katyFix in my codesandbox? Thats missing in otion, its needed for ssr (independed from Deno) 😅

@kripod
Copy link
Owner

kripod commented May 19, 2020

Yes, thank you! I’ll check it out after having lunch.

@katywings
Copy link
Author

2020-05-19 13 17 33

😂

@katywings
Copy link
Author

@kripod Enjoy your lunch!

@kripod
Copy link
Owner

kripod commented May 19, 2020

You too! 😄

kripod added a commit that referenced this issue May 19, 2020
@kripod
Copy link
Owner

kripod commented May 19, 2020

All these issues should be gone with the freshly-released v0.2.2. I've also added instructions about supporting Deno to the core package's readme.

@katywings I'm extremely grateful for all the information you provided. The new version wouldn't have been possible without your help. 🙌

@all-contributors please add @katywings for reporting a bug and the idea about supporting Deno.

@allcontributors
Copy link
Contributor

@kripod

I've put up a pull request to add @katywings! 🎉

@katywings
Copy link
Author

@kripod Thanks a lot for working the Deno things out with me :). Well I am really sorry but I noticed a follow up problem in the new deno builds xD

https://unpkg.com/browse/otion@0.2.4/dist-deno/bundle.dev.d.ts
Looking at the second line import * as CSS from "csstype" Deno will actually try to resolve this import but it can't find 'csstype'.

thread 'main' panicked at 'Can't downcast ErrBox(JSError(JSError { message: "Uncaught Error: Unmapped bare specifier \"csstype\"", sour.....

If you want I can try to figure out from where the csstype thing this is coming tomorrow :).

@kripod
Copy link
Owner

kripod commented May 19, 2020

Thank you for the feedback!

I noticed that recently and updated the Deno setup guide to suggest using Pika CDN, as it correctly resolves csstype as a dependency, based on package.json. Hopefully, other CDNs will catch up with ESM support.

@katywings
Copy link
Author

@kripod Ah nice great docs you got there <3!

Pika CDN is super easy, barely an inconvinience: denoland/deno#5665
🤣

@bebraw
Copy link
Contributor

bebraw commented Sep 10, 2020

Should the current compatibility path work with otion/server as well? I'm trying to get some server-side rendering done with Otion while using Deno environment so I'm curious about this. I can open a separate issue if that's better.

I checked Pika but it looks like they've rebranded themselves as skypack.

In addition the Deno build seems to be the runtime but does it contain the server code as well? Then the question is how to consume that.

@kripod
Copy link
Owner

kripod commented Sep 11, 2020

I haven't been experimenting with Deno SSR, but it should work without issues, as the code doesn't rely upon Node's globals (like process.env.NODE_ENV).

@bebraw
Copy link
Contributor

bebraw commented Sep 12, 2020

@kripod In my use case I'm doing server-side rendering with Otion and I don't see the server specific bits at dist-deno in the distribution. In addition, I use the client side through server as well.

To solve that, I had to tune your runtime code to include a more specific check like this:

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

The process.env check is problem for oceanwind as well and that's why they're using a modified version of Otion for now. I believe that process.env bit may require a more specific check so that you can use the runtime in the browser without any compilation or processing as that's the use case for Oceanwind.

@kripod
Copy link
Owner

kripod commented Sep 12, 2020

@bebraw Interesting, thank you for the insights! I started working on a rewrite of otion, but with a regular daytime job, I'm not sure when I'll have time to execute it. OSS maintenance has become a bit overwhelming for me by now.

@bebraw
Copy link
Contributor

bebraw commented Sep 12, 2020

@kripod Sure, I understand. Let me know if I can be of any help. 👍

I realized that window?.Deno check isn't enough. It should be typeof window !== "undefined" && window.Deno; to protect against Node.

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

No branches or pull requests

3 participants