Custom component manager #16308

smfoote · 2018-03-01T20:49:15Z

Add custom component manager and tests. See #16301 and ember-cli/ember-rfc176-data#43.

rwjblue · 2018-03-02T00:35:15Z

features.json

@@ -8,7 +8,7 @@
    "ember-routing-router-service": true,
    "ember-engines-mount-params": true,
    "ember-module-unification": null,
-    "glimmer-custom-component-manager": null,
+    "glimmer-custom-component-manager": true,


Need to set this back to null to properly test with and without the feature.

rwjblue · 2018-03-02T00:37:07Z

packages/ember-glimmer/lib/component-managers/curly.ts

@@ -198,38 +199,64 @@ export default class CurlyComponentManager extends AbstractManager<ComponentStat
    return { positional: EMPTY_ARRAY, named };
  }

+  /**


Should likely be changed to /* (since /** will be picked up as a YUIDoc comment)

rwjblue · 2018-03-02T00:46:02Z

packages/ember-glimmer/lib/utils/custom-component-manager.ts

+export const COMPONENT_MANAGER = symbol('COMPONENT_MANAGER');
+
+export function componentManager(obj: any, managerId: String) {
+  obj.reopenClass({


Hmmm. I don’t think we need to require reopenClass to be present (and therefore precluding native classes)...

rwjblue · 2018-03-02T00:52:14Z

packages/ember-glimmer/lib/utils/custom-component-manager.ts

+  if (!managerId) { return; }
+
+  let manager = owner.lookup(`component-manager:${managerId}`) as ComponentManager<ComponentStateBucket, DefinitionState>;
+  assert(`Could not find custom component manager '${managerId}'`, !!manager);


Please include the component in the assertion message (otherwise this message isn’t terribly helpful)

rwjblue · 2018-03-02T00:57:00Z

packages/ember-glimmer/lib/index.ts

@@ -300,7 +300,9 @@ export { default as AbstractComponentManager } from './component-managers/abstra
 // rather than the problem was solved
 // DebugStack should just test the assert message
 // it supports for example
-export { UpdatableReference } from './utils/references';
+export { UpdatableReference, RootReference } from './utils/references';


What is RootReference used for outside of ember-glimmer?

rwjblue · 2018-03-02T00:58:08Z

packages/ember-glimmer/lib/index.ts

 export { default as iterableFor } from './utils/iterable';
 export { default as DebugStack } from './utils/debug-stack';
 export { default as OutletView } from './views/outlet';
+export { default as CustomComponentManager } from './component-managers/custom';


Why do we need to export this?

Probably not.

Actually, it's needed in the test file, which does not seem to be able to import from lib.

rwjblue · 2018-03-02T01:01:07Z

packages/ember-glimmer/tests/integration/custom-component-manager-test.js

+     * by passing custom hooks.
+     */
+    registerCustomComponentManager(overrides = {}) {
+      let options = Object.assign({


Needs to be updated to use assign from ember-utils (Object.assign isn’t available on all tested platforms).

rwjblue · 2018-03-02T01:05:24Z

packages/ember-glimmer/tests/utils/helpers.js

@@ -14,5 +14,6 @@ export {
  htmlSafe,
  SafeString,
  DOMChanges,
-  isHTMLSafe
+  isHTMLSafe,
+  COMPONENT_MANAGER


I don’t believe this is needed

rwjblue · 2018-03-02T01:10:03Z

packages/ember-views/lib/system/utils.js

 }

 export function collectChildViews(view, registry) {
  let ids = [];
  let views = [];

-  view[CHILD_VIEW_IDS].forEach(id => {
+  (CHILD_VIEW_IDS.get(view) || []).forEach(id => {


Seems like this should be split up on multiple lines, and in the case where CHILD_VIEW_IDS.get(view) is undefined we can return early (e.g. no need for the forEach)...

rwjblue · 2018-03-02T01:13:56Z

packages/ember/lib/index.js

@@ -439,6 +440,11 @@ Ember.TextField = TextField;
 Ember.TextArea = TextArea;
 Ember.LinkComponent = LinkComponent;

+Object.defineProperty(Ember, '_componentManager', {


Let’s call it setComponentManager here?

sans underscore?

rwjblue · 2018-03-02T18:07:14Z

packages/ember-views/lib/system/utils.js

-  view[CHILD_VIEW_IDS].forEach(id => {
-    let view = registry[id];
+  if (childViews) {
+    (CHILD_VIEW_IDS.get(view) || []).forEach(id => {


I don't think you need to query the weakmap again here, you can reliably do childViews.forEach directly (due to the guard

Shoot - you're totally right. I missed that.

rwjblue · 2018-03-02T18:08:37Z

packages/ember/lib/index.js

@@ -439,6 +440,11 @@ Ember.TextField = TextField;
 Ember.TextArea = TextArea;
 Ember.LinkComponent = LinkComponent;

+Object.defineProperty(Ember, 'setComponentManager', {


darn, I'm sorry for the run around here (I know you implemented exactly what I said in the last comment), I think this still needs to be underscored

Can you change it to be _setComponentManager?

Haha, I did _setComponentManager first, then backed off. Shoulda gone with my gut.

chancancode · 2018-03-02T19:14:03Z

packages/ember-glimmer/lib/utils/custom-component-manager.ts

+    static get [COMPONENT_MANAGER]() {
+      return managerId;
+    }
+  };


Do we want to do this here instead of just mutating the class? The two branches do different things

Are you saying this should be

obj[COMPONENT_MANAGER] = managerId;

instead of returning a new, extended class? That works for me. @rwjblue ?

Yeah, sounds good to me

Also add componentManager to the Ember global for export

rwjblue requested changes Mar 2, 2018

View reviewed changes

rwjblue reviewed Mar 2, 2018

View reviewed changes

chancancode reviewed Mar 2, 2018

View reviewed changes

rwjblue approved these changes Mar 5, 2018

View reviewed changes

tomdale and others added 2 commits March 5, 2018 16:16

Detect custom component manager via installed symbol

7e623f2

Add componentManager method and other methods to delegate interface

8cca013

Also add componentManager to the Ember global for export

smfoote force-pushed the custom-component-manager branch 2 times, most recently from 93a11f3 to 8cca013 Compare March 5, 2018 23:28

rwjblue merged commit caca9d8 into emberjs:master Mar 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom component manager #16308

Custom component manager #16308

smfoote commented Mar 1, 2018

rwjblue Mar 2, 2018

rwjblue Mar 2, 2018

rwjblue Mar 2, 2018

rwjblue Mar 2, 2018

rwjblue Mar 2, 2018

rwjblue Mar 2, 2018

smfoote Mar 2, 2018

smfoote Mar 2, 2018

rwjblue Mar 2, 2018

rwjblue Mar 2, 2018

rwjblue Mar 2, 2018

rwjblue Mar 2, 2018

smfoote Mar 2, 2018

rwjblue Mar 2, 2018

smfoote Mar 2, 2018

rwjblue Mar 2, 2018

smfoote Mar 2, 2018 •

edited

Loading

chancancode Mar 2, 2018

smfoote Mar 5, 2018 •

edited

Loading

rwjblue Mar 5, 2018

Custom component manager #16308

Custom component manager #16308

Conversation

smfoote commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smfoote Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smfoote Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smfoote Mar 2, 2018 •

edited

Loading

smfoote Mar 5, 2018 •

edited

Loading