search

joakin / elm-canvas

A canvas drawing library for Elm
https://package.elm-lang.org/packages/joakin/elm-canvas/latest/
BSD 3-Clause "New" or "Revised" License
163 stars 30 forks source link

Check to make sure window is defined #26

Closed JoshuaHall closed 4 years ago

JoshuaHall commented 4 years ago

This allows the library to be used with server rendering or other situations where window may not be available until runtime. I've been using this library and enjoying it, and this will make it even better since it will allow me to gatsby build my site and deploy it to production without any problems 🙂

joakin commented 4 years ago

The diff on this PR is very dirty. Did you just add the top level if statement?

Another question, this is the JS library for the browser, why would you want to include the file on the server side just to do nothing? Why not just not import it?

And finally, how are you using the library for server side rendering? Elm doesn't support it natively so I'm curious about what you are doing :)

JoshuaHall commented 4 years ago

I did just add the top level if, not sure why the diff looks that way. I guess because of indentation changes?

I'm using GatsbyJS, it's a static site generator that uses React. I'm using a cool plugin (gatsby-plugin-elm) that allows you to include Elm files that will then get compiled and included with your app.

I'm not entirely sure how to include a file that wouldn't be run at build time in Gatsby, I haven't found anything in the docs yet, so I made this PR. I will be making the site open source soonish if you're interested :)

joakin commented 4 years ago

I found the toggle for ignoring whitespace in the diff and it makes more sense now.

Have you read through https://www.gatsbyjs.org/docs/using-client-side-only-packages/ and seen if any of the solutions work for you? It seems like 4) and 2) could be fairly easy to do to just include the script client side. I'm guessing you are doing a normal import and gatsby runs all on the server too.

If you can find an alternate solution it would be appreciated, since the package is just for the browser and it doesn't make much sense to load it on the server, and adding that guard would require to add a comment to explain why it is there and make a new release and look at the docs and links to update them which is non-trivial.

I would definitely be interested to know what you are using the library for!

joakin commented 4 years ago

I have to make a new release of the JS library because of a bug so I'll see if I can include this anyways.

joakin commented 4 years ago

Released 2.2.4 of the JS library on npm which includes this.