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

fix(addStyles): use typeof to check for existence of window && document objects #250

Closed
wants to merge 1 commit into from

Conversation

bherila
Copy link

@bherila bherila commented Jun 22, 2017

What kind of change does this PR introduce?

Fixes the following error which occurs during Webpack build by adding the check via the typeof operator.

        return window && document && document.all && !window.atob;
               ^

ReferenceError: window is not defined
    at C:\Users\ucadmin\Documents\uc\admin\build\server.js:32858:9

Did you add tests for your changes?
n/a

If relevant, did you update the README?
n/a

Summary

Of note, other places in this file use the typeof operator, but curiously it is not done here.

Does this PR introduce a breaking change?
No

Other information
n/a, it's a one-liner 😄

@jsf-clabot
Copy link

jsf-clabot commented Jun 22, 2017

CLA assistant check
All committers have signed the CLA.

@bherila
Copy link
Author

bherila commented Jun 22, 2017

FYI this is in addition to #177 which curiously is not enough 😦

@michael-ciniawsky michael-ciniawsky changed the title Use typeof to check for existence of window and document objects fix(addStyles): use typeof to check for existence of window && document objects Jun 22, 2017
@michael-ciniawsky michael-ciniawsky self-assigned this Jun 22, 2017
@michael-ciniawsky michael-ciniawsky added this to the 0.18.3 milestone Jun 22, 2017
@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Jun 22, 2017

@bherila In which exact enviroment do you use the loader (JSDOM) ?

Copy link
Contributor

@bebraw bebraw left a comment

Choose a reason for hiding this comment

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

Seems sane to me. Maybe it's better to push the check to one place, though, and use it across the codebase, though?

@alexander-akait
Copy link
Member

@bherila can your provide use case, where your need this check? best - create test repo

@bherila
Copy link
Author

bherila commented Jun 22, 2017

@michael-ciniawsky It was in my attempt to get https://github.com/barbar/vortigern working with bootstrap-loader (bootstrap 4). I have been attempting to get bootstrap 4 rendering with isomorphic-style-loader, and ideally have the bootstrap 4 critical path CSS analyzed and rendered server-side. As you can see from the vortigern site, there are a lot of packages being assembled together here :-\

Also, I would have shared the repo in which this reproduces, however it's for a client and I am not sure I can publish all the code on Github. I can try to make a minimal repro-repo if it would be helpful somehow. It's basically the stock vortigern + a few source files + bootstrap-loader.

@veeramarni
Copy link

any update on merging this issue.

@michael-ciniawsky
Copy link
Member

@veeramarni Could you describe your 'use case' ? Why/Where are the current checks insufficient?

⚠️ I'm inclined to close this PR within the next 48 hours in case no concrete reproduction is provided

@veeramarni
Copy link

veeramarni commented Nov 9, 2017

@michael-ciniawsky I still see those errors when running with SSR. In my case, we use webpack to create npm packages and we use these packages in an application where we get window is not defined error when we try to run with SSR. This could be due to the packaged webpack build has the style-loader.

@michael-ciniawsky
Copy link
Member

SSR isn't officially supported, this loader only works in the browser or browser-like env like JSDOM.

we get window is not defined

Would this PR resolve your issue ?

@veeramarni
Copy link

I will give a try with this PR and let you know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants