-
-
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
Refactor template factory #18096
Refactor template factory #18096
Conversation
83cbc0e
to
5ece62b
Compare
Failed on linting:
Need to fix this one manually I think. |
5ece62b
to
4418283
Compare
I just rebased and fixed the linting failure. @pzuraq / @krisselden - I'd love review here when y'all have a minute. |
4418283
to
3bbc08d
Compare
Per RFC #481, we want to align the factory signature between templates and component managers. Since the factories for component managers are just a function that takes the owner, we decided to move template factories in the same direction here as well. Co-authored-by: Robert Jackson <me@rwjblue.com>
CI is green now, still needs some review though... Ping again @krisselden / @pzuraq 😸 |
registry.register('view:-outlet', OutletView); | ||
registry.register('template:-outlet', OutletTemplate); | ||
registry.register('template:-outlet', OutletTemplate as any); |
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.
why does this need as any
?
{ | ||
componentDefinitionCount: 1, | ||
// 1 from this.render, 1 from component-one | ||
templateCacheMisses: 2, |
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.
prior to this change, this path was only used for late bound templates that were cached because their lifetime wasn't in the DI system's container.
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.
Yep, thats correct. Are you saying this is bad/wrong/etc?
emberjs/ember.js#18096 This PR changed how templates get transpiled under the hood. The export from templates are now factory functions which expect to be passed the `owner`.
emberjs/ember.js#18096 This PR changed how templates are used under the hood. Template modules now export a factory function that expects to be called with the `owner`.
ember.js/packages/@ember/-internals/glimmer/lib/component-managers/outlet.ts Lines 72 to 74 in 099aec5
if (DEBUG) {
let owner = getOwner(this);
let template = definition.template(owner);
this._pushToDebugStack(`template:${template.referrer.moduleName}`, environment);
} Or should this class actually still be instantiated with an |
emberjs/ember.js#18096 This PR changed how templates are used under the hood. Template modules now export a factory function that expects to be called with the `owner`.
Example of a test suite failure: https://travis-ci.org/salsify/ember-css-modules/jobs/552819509 |
|
@rwjblue Thanks for the pointer! I submitted a PR: emberjs/ember-test-helpers#677 |
#18168 should fix the specific issue that @buschtoens ran into (without necessarily coupling it to an |
emberjs/ember.js#18096 This PR changed how templates get transpiled under the hood. The export from templates are now factory functions which expect to be passed the `owner`.
Per RFC #481, we want to align the factory signature between templates and component managers. Since the factories for component managers are just a function that takes the owner, we decided to move template factories in the same direction here as well.