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

remove dependency on 'window', declare missing variable #23

Merged
merged 2 commits into from
Dec 14, 2015
Merged

remove dependency on 'window', declare missing variable #23

merged 2 commits into from
Dec 14, 2015

Conversation

sedenardi
Copy link
Contributor

In order to make this plugin work with server-side rendering, I've removed the dependency on window by declaring it when it detects a server environment (when module is declared) and skips the hasOwnProperty call when it is undefined.

I've also declared tRatio which was previously just using a global variable.

@sedenardi
Copy link
Contributor Author

Also, have you considered publishing this lib to npm?

@aullman
Copy link
Owner

aullman commented Dec 14, 2015

Thanks @sedenardi. It is on npm https://www.npmjs.com/package/opentok-layout-js

aullman pushed a commit that referenced this pull request Dec 14, 2015
remove dependency on 'window', declare missing variable
@aullman aullman merged commit 8fae672 into aullman:master Dec 14, 2015
@aullman
Copy link
Owner

aullman commented Dec 14, 2015

What server-side rendering are you using this for btw? What's the use-case? Is this for archiving? I am interested.

@sedenardi
Copy link
Contributor Author

@aullman Thanks for this.

I'm using Nodejs + Reactjs in my application. I render the markup on the server so that there's no perceivable load times to the user (as opposed to most single-page applications where you send the markup for a container and the page is only fully loaded once all the JS files are downloaded). For this and other 3rd party libraries, I have to make safe references to window because window doesn't exist when rendering on the server, only document does. This usually isn't an issue, as most libs only do DOM manipulation and don't write global variables.

@aullman
Copy link
Owner

aullman commented Dec 14, 2015

Ah, that makes sense, cool, thanks.

@sedenardi
Copy link
Contributor Author

@aullman Note, b7d1b5c broke this fix, as window is now undefined on the server again.

I'm getting around this by adding a shim before requiring the lib:

if (!global.window) { global.window = null; }
var OpenTokLayout = require('opentok-layout-js');

Not a bug per se, but thought you should be aware (and maybe add a note in the README, but not a huge deal).

@aullman
Copy link
Owner

aullman commented Jun 8, 2016

Bummer, sorry about this. Feel free to submit a PR that fixes this issue. The problem I had was that IE10 was complaining when I was defining a window variable even though window wasn't getting used in IE10 context.

Actually a quick fix would just be to call it win instead of window, I might do that now. I should probably also add a test that includes the file in node to make sure it's working.

@aullman aullman mentioned this pull request Jun 8, 2016
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.

None yet

2 participants