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 basic Node.js rendering #10184

Merged
merged 1 commit into from
Jan 15, 2015
Merged

Support basic Node.js rendering #10184

merged 1 commit into from
Jan 15, 2015

Conversation

tomdale
Copy link
Member

@tomdale tomdale commented Jan 10, 2015

This commit makes some small cleanup changes across the framework that
ensure that views’ renderers are consistently inherited from their
parents (so that, e.g., if you render a view using a Node renderer, all
subviews will use that same renderer).

It also changes one instance where DOM was being relied upon rather than
using the DOMHelper abstraction.

It also includes a small suite of tests that manually patch views to use
the Node SSR renderer, verifying that compiled HTMLbars templates render
correctly.

@@ -3,19 +3,148 @@
var path = require('path');
var distPath = path.join(__dirname, '../../dist');

QUnit.config.notrycatch = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if you want to keep this in here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great catch; I did not mean to keep that.

Let me take this opportunity to rant about the fact that you can have a completely wrong set of npm packages installed that do not satisfy the dependencies in your package.json and neither node nor npm will do anything to warn you, just choosing to require the wrong package. This is debugging detritus from us trying to explain why the tests were passing on one computer but inexplicably not passing on another, and it ended up being us forgetting to npm install. Isn't this what computers are supposed to be good at?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea NPM isn't that great. The other issue is that not everyone strictly follows semver, so stupid ranged version numbers can bite you as well. Because of this Ember CLI now adds --save-exact on anything it installs.

@chadhietala
Copy link
Contributor

This is awesome. 👍

@@ -64,6 +64,7 @@ function appendBlockConditional(view, inverted, helperName, params, hash, option
view.appendChild(BoundIfView, {
_morph: options.morph,
_context: get(view, 'context'),
renderer: get(view, 'renderer'),
Copy link
Member

Choose a reason for hiding this comment

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

Can this be view.renderer? And why is the renderer not set in the other views, e.g. in the with helper?

Copy link
Member

Choose a reason for hiding this comment

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

I wonder why we need to thread it through like this also. I thought perhaps it could be added at a higher level (to prevent having to sprinkle the wiring through everywhere). Possibly like adding to 'appendChild'?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, agreed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds like an awesome suggestion. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

@mmun Why is _context hardcoded here and not in appendChild?

Copy link
Member Author

Choose a reason for hiding this comment

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

createChildView feels like a better place for this than appendChild, what say you all?

@tomdale
Copy link
Member Author

tomdale commented Jan 13, 2015

@chadhietala @mmun Updated based on your feedback, thanks!

@tomdale tomdale force-pushed the pass-parent-renderer branch 2 times, most recently from 1088a0f to 837447f Compare January 13, 2015 01:52
ok(serializer.serialize(morph.element).match(/<h1>Hello World<\/h1>/));
});

QUnit.test("It is possible to render a view with an {{#if}} helper in Node", function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

"It is possible to render a {{view}} helper in Node"?

This commit makes some small cleanup changes across the framework that
ensure that views’ renderers are consistently inherited from their
parents (so that, e.g., if you render a view using a Node renderer, all
subviews will use that same renderer).

It also changes one instance where DOM was being relied upon rather than
using the DOMHelper abstraction.

It also includes a small suite of tests that manually patch views to use
the Node SSR renderer, verifying that compiled HTMLbars templates render
correctly.
mmun added a commit that referenced this pull request Jan 15, 2015
@mmun mmun merged commit 7e16727 into master Jan 15, 2015
@mmun mmun deleted the pass-parent-renderer branch January 15, 2015 00:06
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.

5 participants