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

importing metal-dom shouldn't immediately throw errors when running in Node.js environment #264

Merged
merged 3 commits into from
Oct 11, 2017

Conversation

robframpton
Copy link

Hey @jbalsas

This is just a step towards out of the box isomorphic rendering. With these changes you'll be able to have an import to metal-dom in a Node.js environment without it immediately throwing an error.

@@ -32,6 +32,9 @@ class features {
const prefixes = ['Webkit', 'MS', 'O', ''];
const typeTitleCase = string.replaceInterval(type, 0, 1, type.substring(0, 1).toUpperCase());
const suffixes = [`${typeTitleCase}End`, `${typeTitleCase}End`, `${typeTitleCase}End`, `${type}end`];
if (!features.animationElement_) {
features.animationElement_ = document.createElement('div');
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @Robert-Frampton, won't this still throw on usage on the server?

Copy link
Author

Choose a reason for hiding this comment

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

Only if checkAnimationEventName_ actually get's invoked, but it should never get invoked on the server. That's why the custom events are only being registered if it's not server side.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I was thinking about other usages not protected here, like https://github.com/metal/metal-anim/blob/master/src/Anim.js#L60

Copy link
Author

Choose a reason for hiding this comment

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

I think we might just need to state that some packages are not supported in server side rendering. All of the methods in metal-anim require DOM elements, so you really shouldn't be invoking on the server anyways.

Copy link
Author

Choose a reason for hiding this comment

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

Also, even if a component implemented metal-anim in some way, it probably isn't going to be invoked by renderToString.

@robframpton robframpton changed the base branch from master to develop October 10, 2017 16:24
@jbalsas jbalsas merged commit b8e1755 into metal:develop Oct 11, 2017
@jbalsas jbalsas added this to the 2.14.0 milestone Oct 11, 2017
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.

2 participants