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

Proposal: update browser check to look for document #549

Closed
alexlafroscia opened this issue Jan 28, 2018 · 7 comments
Closed

Proposal: update browser check to look for document #549

alexlafroscia opened this issue Jan 28, 2018 · 7 comments

Comments

@alexlafroscia
Copy link
Contributor

I've been working on an Ember compatibility library for emotion (ember-emotion) and have been trying to get server-side rendering working by integrating with FastBoot. However, I've been having a bit of trouble because of the way that emotion checks for the browser environment.

In FastBoot, window is defined but document is not. emotion checks for window but then never uses it; it uses document in a number of places to inject the styled into the page.

Is there a reason for this? If not, would you be willing to accept a PR that checks for the presence of document instead, since that's the actual API that emotion depends on?

If you'd like to maintain the check against window for browser-environment detection, would you be willing to explore the ability to inject the reference to document so that it could be overwritten in a case like mine where document isn't available?

@alexlafroscia
Copy link
Contributor Author

My progress toward supporting server-side rendering can be found in this PR:

alexlafroscia/ember-emotion#16

Although I had to modify this line to check for typeof window !== 'undefined' && typeof document !== 'undefined

this.isBrowser = typeof window !== 'undefined'

@alexlafroscia
Copy link
Contributor Author

alexlafroscia commented Jan 29, 2018

I've identified two locations in the 9.0.0-2 code that I had to modify to make my SSR solution work with the new version of emotion:

https://github.com/emotion-js/emotion/blob/master/packages/create-emotion/src/index.js#L315

https://github.com/emotion-js/emotion/blob/master/packages/create-emotion/src/utils.js#L74

I think that the first one referenced should import the isBrowser util instead of defining that comparison a second time, and then the util should check window and document.

I'm happy to make this PR myself, just want the go-ahead that you're interested in these changes and that there isn't some additional context around why checking only window is necessary.

@emmatown
Copy link
Member

emmatown commented Jan 29, 2018

There's no special reason we check for window, I think we don't need to check for window and document, we can just check for document.

@alexlafroscia
Copy link
Contributor Author

Awesome! In that case, I’ll make a PR with my proposed changes. Thanks @mitchellhamilton

alexlafroscia pushed a commit to alexlafroscia/emotion that referenced this issue Jan 29, 2018
alexlafroscia added a commit to alexlafroscia/emotion that referenced this issue Jan 29, 2018
emmatown pushed a commit that referenced this issue Jan 30, 2018
@alexlafroscia
Copy link
Contributor Author

@mitchellhamilton is there a timeline for when emotion@9 will be released? Or the next beta, at least? I'm excited to release my next version of ember-emotion but need the change I landed here first.

@emmatown
Copy link
Member

emmatown commented Feb 3, 2018

@alexlafroscia Just published [email protected] on the next tag

@alexlafroscia
Copy link
Contributor Author

woot! thank you!

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

No branches or pull requests

2 participants