Skip to content
This repository has been archived by the owner on Sep 9, 2022. It is now read-only.

Why is Modifier.modifier necessary? #7

Closed
chancancode opened this issue Jul 3, 2019 · 10 comments
Closed

Why is Modifier.modifier necessary? #7

chancancode opened this issue Jul 3, 2019 · 10 comments

Comments

@chancancode
Copy link
Collaborator

The intent is that this addon will set it on the super class that your users extend from, and Ember will walk the class chain and find the manager. Is there some reason why that didn't work out?

@sukima
Copy link
Owner

sukima commented Jul 8, 2019

@chancancode IIRC (because it has been so long) the documentation (lack of) specifically needed to call Ember._setModifierManager with a factory method and a class reference (not instance). Presumably this is so that Ember can instantiate when it needs to. Therefor the best API I could think of at the time was to provide factory methods for the two classes that know how to call Ember._setModifierManager

If you have a suggestion feel free to offer it. I by no means am an authority on Object Oriented Programming. I just feel passionate about OO because of amazing work from people like Sandi Metz, Bob Martin, Martin Fowler, etc. I would hate to see all that effort lost in a frantic Functional crazed ecosystem. Hence why I ran with this addon idea.

@sukima
Copy link
Owner

sukima commented Jul 8, 2019

Another way of putting it (I'm asking for help and guidence on this):

I don't fully understand the question. I was under the impression that the Factory method pattern was a well established Object Oriented Pattern. According to the documentation at the time Ember._setModifierManager needs to be called with a Class not instance making its use in a contructor invalid necessitating a factory.

@chancancode
Copy link
Collaborator Author

Yes I appreciate it, thank you for the work! :)

Yes you would call setModifierManager on a class not an instance. However, if a manager is not found on the exported class (from app/modifiers/*.js), Ember will walk its superclass chain (prototype chain) until it finds one, or else it will error. Since, in your OO manager, everyone subclasses from the classes you provided, you can simply call that on your provided superclass, and every user written modifiers will have the write manager set.

Technically, the exported value (from app/modifiers/*.js) doesn’t have to be a class, but nevertheless ember still expect that value or somewhere along its prototype chain has a manager set, that’s how we provide flexibility for things like functions. However, the OO use case is definitely the first-class one in our design, so you can take advantage of that and make the api less verbose for the users.

@sukima
Copy link
Owner

sukima commented Jul 9, 2019

I think the confusion comes down to a miscommunication here. How does a super class know what child is to be used in the setModifierManager call if modifier is a static method? You mentioned prototype chains but that only works for instances and in the case of setModifierManager we are talking classes not instances.

I know I am missing some kind of meta-programming here but I don't recall which. Considering that the functional implementation was the only documentation at the time. Maybe an explanation on what setModifierManager is and how it is expected to be used? Any chance for some code examples?

I would love to remove the .modifier() call. I just don't understand how based on the mental model I have so far.

Since, in your OO manager, everyone subclasses from the classes you provided, you can simply call that on your provided superclass

Who calls setModifierManager? Is this in the constructor? An inherited static method? If I pass a reference to the parent class to setModifierManager will it still work even though the user wants their own subclass of the parent?

I am really sorry for my misunderstandings.

@chancancode
Copy link
Collaborator Author

Concretely, I am suggesting that:

  1. In the addon code, you can call setModifierManager(createManager, Modifier)
  2. Modifier.modifier can be made into a no-op function, deprecated and eventually removed (the app should not need to call it)
  3. In app code, it should just be export default Modifier.extend({ ... } or export default class extends Modifier { ... }.

This creates a "subclass" of the Modifier base class from this addon, which has the manager set in (1). When Ember look at app's modifier class from (3), it will first check if that class has a manager set (it does not), then it will check if its super class has the manager set. Since the super class is the Modifier base class from this addon, which does have the manager set in step (1), everything will work correctly.

The way this works in each class has a prototype pointer (Object.getPrototypeOf) into its super class. The class in turns as a prototype pointer into its super class. Eventually, you will reach the top and Object.getPrototypeOf will return null. This is essentially how inheriting static methods work. Ember follows the same chain to find the manager.

madnificent added a commit that referenced this issue Jul 10, 2019
It was previously necessary to call Modifier.modifier on the exported
modifier class.  We can remove this requirement based on the input of
@chancancode in #7.  This commit implements the behaviour as we
understood it from this issue.

The deprecation message indicates version 1.0.0 will drop support for
calling Modifier.modifier, this has not been discussed and is subject
to change.

The examples of the README have been updated to not show the
deprecated Modifier.modifier call.

The tests have been duplicated to contain both the calls with
Modifier.modifier as well as those without.  The tests have been
updated mechanically with Emacs Macro's rather than being smart about
them to keep the tests obvious.
madnificent added a commit that referenced this issue Jul 10, 2019
It was previously necessary to call Modifier.modifier on the exported
modifier class.  We can remove this requirement based on the input of
@chancancode in #7.  This commit implements the behaviour as we
understood it from this issue.

The deprecation message indicates version 1.0.0 will drop support for
calling Modifier.modifier, this has not been discussed and is subject
to change.

The examples of the README have been updated to not show the
deprecated Modifier.modifier call.

The tests have been duplicated to contain both the calls with
Modifier.modifier as well as those without.  The tests have been
updated mechanically with Emacs Macro's rather than being smart about
them to keep the tests obvious.
@sukima
Copy link
Owner

sukima commented Jul 10, 2019

Now I understand the confusion. ModifierA extends Modifier. We have registered Modifier not ModifierA. Ember walks up the prototype chain and finds Modifier. But it now has Modifier not ModifierA. My confusion is that by stating setModifierManager(..., Modifier) it is not ModifierA's manager. it is the parents manager. So the notion that we are setting the parent not the child is confusing as all heck.

It would seem based on what you are saying is that we are not setting the modifier manager like the method name suggests but instead registering some kind of magical suggestion. This somehow does something that is not setting and the user's manager is used despite never having registered it but merely its parent class.

I appreciate your explanation and based on PR #8 working this magic works. I'll go with it but I am still missing one key piece of information to understand it.

@chancancode
Copy link
Collaborator Author

If it helps you, the thing this is modeling is this:

class Parent {}
Parent.manager = MyManager;


class Child extends Parent {}
Child.manager // => MyManager

The prototype chain is just describing how this mechanism works under the good in vanilla JavaScript, but inheriting static properties are common and expected. The only reason a weak map is used instead of a property is (first and foremost an implementation detail) to avoid “polluting” the class when you look at it from the console or when looping through it’s properties.

@sukima
Copy link
Owner

sukima commented Jul 10, 2019

Parent.manager = MyManager;

So the user has to do this? According to the above it is more like: Parent.manager = Parent; Which is why I'm confused.

@sukima
Copy link
Owner

sukima commented Jul 11, 2019

After much discussion with @madnificent I think I finally figured out where my mental model was blocked. I going to re-iterate my understanding here for prosperity.

First, this concept is based on the idea that duck typing is not used. This is where my mental model could not understand because I was hard pressed to think that if a ModifierManager quacks like a ModifierManager then it was a ModifierManager. However, if duck typing were removed from the mental model then there would be need to have some way to type check that a ModifierManager subclass inherits from a known ModifierManger.

The chain of events is:

  1. An addon author makes a parent ModifierManager class
  2. They then register that class with setModifierManager(factoryMethod, ParentModelManagerKlass)
  3. Ember takes that Klass and uses it as a reference in its known manager list (so to speak)
  4. When Ember later encounters a possible ModifierManager subclass it walks its prototype chain checking each step to see if one of its parent classes is in that known manager list.
  5. This verification walk allows Ember to consider the original instance the user created as a valid ModifierManager and not a goose disguised as a duck.

Even though setModifierManager takes a class it is not the class that is created via the factory method but used just to type check in a non duck type way.

Internally it uses a WeakMap to avoid holding onto references to Classes but the consequence of doing that is that Ember has to implement its own prototype walk and can not use instanceof like most would expect.

I think this is how the world works. I hope. cheesy-grin

@chancancode
Copy link
Collaborator Author

This has been fixed in #8!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants