Skip to content

Feature: enable server-side-rendering #17

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

Merged
merged 3 commits into from
Mar 7, 2018

Conversation

brettgullan
Copy link
Contributor

Added a couple of fixes to allow component to be server rendered.

  • fixed: Picturefill import throwing error on import
  • fixed: ieVersion check throwing error referencing window.document

Copy link
Owner

@braposo braposo left a comment

Choose a reason for hiding this comment

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

hey @brettgullan first of all a massive thanks for taking the time to work on this and I'm really sorry I didn't reply earlier! I've been focusing on other work and totally missed this PR 🙏

Overall it looks fine, I don't really have any case where we're using SSR so can't test but it makes sense so I assume it's working.

I left a comment on the can-use-dom import that I'd appreciate your opinion. Once it's cleared we can merge this and release a new version.

Sorry again for the delay, hope it wasn't blocking anything on your side!

import glamorous from "glamorous";
import canUseDom from 'can-use-dom'
Copy link
Owner

Choose a reason for hiding this comment

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

Given that can-use-dom is literally a function, what's your opinion on adding a dependency vs just defining that function in the library?

I know that npm is much more secure now and the chances something goes wrong are small, but given that the function defined isn't really something that we expect to change in the future I was more on the side of defining the function in our code instead of importing another dependency.

@brettgullan
Copy link
Contributor Author

Hey @braposo,

I don't really have a strong opinion either way regarding adding the dependency vs. defining the function.
That said, I have tended towards the habit of trying to externalize functions such as this, that otherwise end up being repeated time and again, across different modules.

While adding multiple dependencies such as this is a slight drawback for now, my thinking was more that stuff like this should really end up in a utility-belt module/library, similar to Lodash or Ramda.

So, on balance, I went with the dependency, rather than the code duplication.

Your call, though … happy to change if you'd prefer.

Cheers,

Brett

@braposo
Copy link
Owner

braposo commented Mar 7, 2018

Sorry @brettgullan I just forgot about this... Let's merge it for now and I'll deal with the extra dependency later. I'm doing some improvements anyway so I'll change it if needed.

Thanks again for your help, I'm sorry it took so long to merge this! 🙏

@braposo braposo merged commit ce18916 into braposo:master Mar 7, 2018
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.

2 participants