Skip to content

Avoid fails from serverside renderings #3909

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.

Sign up for GitHub

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 1 commit into from
Mar 21, 2017
Merged

Avoid fails from serverside renderings #3909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/chart.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,7 @@ plugins.push(require('./plugins/plugin.filler.js')(Chart));

Chart.plugins.register(plugins);

window.Chart = module.exports = Chart;
module.exports = Chart;
if (typeof window !== 'undefined') {
window.Chart = Chart;
}
5 changes: 5 additions & 0 deletions src/core/core.helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,11 @@ module.exports = function(Chart) {
};
// Request animation polyfill - http://www.paulirish.com/2011/requestanimationframe-for-smart-animating/
helpers.requestAnimFrame = (function() {
if (typeof window === 'undefined') {
Copy link
Member

@etimberg etimberg Mar 18, 2017

Choose a reason for hiding this comment

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

I'm not sure how I feel about this change. I understand that it's rendering the chart synchronously if window is not defined, but that can be achieved by setting the animation duration to 0. @simonbrunel thoughts?

Copy link
Member

@simonbrunel simonbrunel Mar 19, 2017

Choose a reason for hiding this comment

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

I think this use case (server side rendering) should rely on a new platform (e.g. platform.node) but not sure how it would be implemented. For now it may be easier to merge these (harmless) changes if it helps projects to integrate Chart.js.

@khorolets are these the only checks needed in order to have Chart.js running server side? What about the other specific HTMLCanvasElement, CanvasRenderingContext2D, CanvasGradient or even the iframe?

Copy link

@frol frol Mar 19, 2017

Choose a reason for hiding this comment

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

We (@khorolets and I) don't use Chart.js to render charts on the server, that being said, we wrap the code calling Chart.js rendering into guards to avoid the execution on the server. HOWEVER, without this patch, we cannot have Chart.js in our bundle because Chart.js initializes some of its parts right away.

Copy link

@frol frol Mar 19, 2017

Choose a reason for hiding this comment

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

What about the other specific HTMLCanvasElement, CanvasRenderingContext2D, CanvasGradient or even the iframe?

Since we don't encounter any other problems, I believe those parts are not triggered on script evaluation, and that is fine with our server side.

Copy link
Member

@simonbrunel simonbrunel Mar 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Makes sense, can you rebase and resolve conflicts?

Copy link

@frol frol Mar 19, 2017

Choose a reason for hiding this comment

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

@simonbrunel I have already asked @khorolets to do that in a private chat, but I believe he is already offline for today, so we will address this the first thing tomorrow morning.

Copy link
Member

@simonbrunel simonbrunel Mar 19, 2017

Choose a reason for hiding this comment

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

@etimberg are you good to merge those changes? We can still plan a better handling of window by moving all references into the dom platform.

Copy link
Member

@etimberg etimberg Mar 19, 2017

Choose a reason for hiding this comment

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

I am good with these changes.

Copy link
Contributor Author

@khorolets khorolets Mar 20, 2017

Choose a reason for hiding this comment

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

@simonbrunel I've resolved the conflicts.

return function(callback) {
callback();
};
}
return window.requestAnimationFrame ||
window.webkitRequestAnimationFrame ||
window.mozRequestAnimationFrame ||
Expand Down