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

support server side rendering #461

Closed
wants to merge 1 commit into from
Closed

Conversation

simoneb
Copy link

@simoneb simoneb commented Feb 8, 2017

When using in server side rendering scenarios (i.e using React) this package once required will throw an exception because the window object is not defined.

This fix prevents both host and guest scripts from doing anything if the window object is not defined, preserving the same behavior when used client side.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 94.903% when pulling 38b876a on simoneb:master into 6f0a2c1 on davidjbradshaw:master.

@@ -1101,4 +1103,4 @@



})(window || {});
})(typeof window !== 'undefined' && window);
Copy link
Owner

Choose a reason for hiding this comment

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

Couldn't this line now just be this?

})(window)

Copy link
Author

@simoneb simoneb Feb 8, 2017

Choose a reason for hiding this comment

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

no, it will throw an exception because the global variable window is not defined. I got inspiration from this. But I don't think we need that many checks here

@davidjbradshaw
Copy link
Owner

I've merged this in by hand, as you added it to the code in js rather than src.

Thanks.

@simoneb
Copy link
Author

simoneb commented Feb 17, 2017

sorry but the merge you did is wrong. it really needs to be

typeof window !== 'undefined' && window

accessing the window object if it's undefined will throw and exception, whereas && will shortcircuit and won't throw an exception

@davidjbradshaw
Copy link
Owner

OK, so simplifying to just not pass in window

;(function(undefined) {
	'use strict';

	if(typeof window === 'undefined') return; // don't run for server side render

...

})()

Is that ok?

@simoneb
Copy link
Author

simoneb commented Feb 20, 2017

it's a weird way to do that but as long as you don't try to access the window global variable before checking if its type is undefined it should be fine.

Out of curiosity, what was wrong with the implementation in my PR?

@davidjbradshaw
Copy link
Owner

davidjbradshaw commented Feb 20, 2017 via email

@simoneb
Copy link
Author

simoneb commented Feb 20, 2017

I would just port the change as it is, even though it was done to the wrong files its semantics still apply.

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.

3 participants