-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Conversation
@@ -3,19 +3,148 @@ | |||
var path = require('path'); | |||
var distPath = path.join(__dirname, '../../dist'); | |||
|
|||
QUnit.config.notrycatch = true; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
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'), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, agreed.
There was a problem hiding this comment.
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. 👍
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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?
da7af43
to
0b09246
Compare
@chadhietala @mmun Updated based on your feedback, thanks! |
1088a0f
to
837447f
Compare
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() { |
There was a problem hiding this comment.
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"?
837447f
to
4dd2c4f
Compare
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.
4dd2c4f
to
80fcde8
Compare
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.