Skip to content

Refactor template factory#18096

Merged
rwjblue merged 1 commit into
masterfrom
refactor-template-factory
Jun 27, 2019
Merged

Refactor template factory#18096
rwjblue merged 1 commit into
masterfrom
refactor-template-factory

Conversation

@chancancode
Copy link
Copy Markdown
Member

@chancancode chancancode commented Jun 13, 2019 •
edited
Loading

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.

@wycats wycats force-pushed the refactor-template-factory branch from 83cbc0e to 5ece62b Compare June 13, 2019 19:45
@rwjblue rwjblue requested review from krisselden and pzuraq June 13, 2019 19:45
@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Jun 13, 2019 •
edited
Loading

Failed on linting:

$ tsc --noEmit
packages/@ember/-internals/glimmer/lib/component-managers/root.ts(27,22): error TS2554: Expected 1 arguments, but got 2.
error Command failed with exit code 1.

Need to fix this one manually I think.

@rwjblue rwjblue force-pushed the refactor-template-factory branch from 5ece62b to 4418283 Compare June 14, 2019 19:19
@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Jun 14, 2019

I just rebased and fixed the linting failure.

@pzuraq / @krisselden - I'd love review here when y'all have a minute.

@rwjblue rwjblue force-pushed the refactor-template-factory branch from 4418283 to 3bbc08d Compare June 17, 2019 19:47
@chancancode @rwjblue
Refactor template factory
3bbc08d 
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>
@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Jun 18, 2019

CI is green now, still needs some review though...

Ping again @krisselden / @pzuraq 😸

krisselden
krisselden reviewed Jun 23, 2019
View reviewed changes
Comment thread packages/@ember/-internals/glimmer/lib/setup-registry.ts

registry.register('view:-outlet', OutletView);
registry.register('template:-outlet', OutletTemplate);
registry.register('template:-outlet', OutletTemplate as any);
Copy link
Copy Markdown
Contributor

@krisselden krisselden Jun 23, 2019

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?

krisselden
krisselden reviewed Jun 23, 2019
View reviewed changes
Comment thread packages/@ember/-internals/glimmer/tests/unit/runtime-resolver-cache-test.js
{
componentDefinitionCount: 1,
// 1 from this.render, 1 from component-one
templateCacheMisses: 2,
Copy link
Copy Markdown
Contributor

@krisselden krisselden Jun 23, 2019

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.

Copy link
Copy Markdown
Member

@rwjblue rwjblue Jun 23, 2019

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?

@rwjblue rwjblue merged commit 321370e into master Jun 27, 2019
@rwjblue rwjblue deleted the refactor-template-factory branch June 27, 2019 17:04
buschtoens added a commit to ember-decorators/ember-decorators that referenced this pull request Jul 1, 2019
@buschtoens
fix(component): @layout accept template factory
816c00c 
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`.
@buschtoens buschtoens mentioned this pull request Jul 1, 2019
Closed
buschtoens added a commit to buschtoens/ember-css-modules that referenced this pull request Jul 1, 2019
@buschtoens
fix: support template factory
456bc34 
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`.
@buschtoens buschtoens mentioned this pull request Jul 1, 2019
Merged
@buschtoens
Copy link
Copy Markdown
Contributor

buschtoens commented Jul 1, 2019 •
edited
Loading

ember.js/packages/@ember/-internals/glimmer/lib/component-managers/outlet.ts

Lines 72 to 74 in 099aec5

if (DEBUG) {
this._pushToDebugStack(`template:${definition.template.referrer.moduleName}`, environment);
}

definition.template is a Factory now and this breaks. Where is the error? Should this actually be typed and used as a Factory, which would entail changes in quite a few files, e.g.:

    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 OwnedTemplate, like before?

buschtoens added a commit to buschtoens/ember-css-modules that referenced this pull request Jul 1, 2019
@buschtoens
fix: support template factory
42fc271 
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`.
@buschtoens
Copy link
Copy Markdown
Contributor

buschtoens commented Jul 1, 2019

Example of a test suite failure: https://travis-ci.org/salsify/ember-css-modules/jobs/552819509

@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Jul 2, 2019

@ember/test-helpers needs a corresponding change (with feature detection) to avoid the error. FWIW, this is why we did this change early in the canary cycle so we can shake out any issues prior to beta.

buschtoens added a commit to buschtoens/ember-test-helpers that referenced this pull request Jul 2, 2019
@buschtoens
fix(setupRenderingContext): compatibility with template factory
ee1d91e 
emberjs/ember.js#18096 (comment)
@buschtoens buschtoens mentioned this pull request Jul 2, 2019
Merged
@buschtoens
Copy link
Copy Markdown
Contributor

buschtoens commented Jul 2, 2019

@rwjblue Thanks for the pointer! I submitted a PR: emberjs/ember-test-helpers#677

buschtoens added a commit to buschtoens/ember-test-helpers that referenced this pull request Jul 2, 2019
@buschtoens
fix(ModuleForComponent): compatibility with template factory
7227929 
emberjs/ember.js#18096 (comment)
@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Jul 3, 2019

#18168 should fix the specific issue that @buschtoens ran into (without necessarily coupling it to an @ember/test-helpers update).

@kategengler kategengler mentioned this pull request Jul 3, 2019
Closed
15 tasks
@buschtoens buschtoens mentioned this pull request Jul 3, 2019
Merged
josemarluedke pushed a commit to josemarluedke/ember-decorators that referenced this pull request Aug 12, 2019
@buschtoens @josemarluedke
fix(component): @layout accept template factory
c78a5ed 
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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rwjblue rwjblue rwjblue left review comments

@pzuraq pzuraq Awaiting requested review from pzuraq

+1 more reviewer

@krisselden krisselden krisselden left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chancancode @rwjblue @buschtoens @krisselden