Merge with ember-class-based-modifier

[chancancode]

Hello!

While waiting for #13 to be merged and released (it's blocking our 3.13 upgrade at work), I decided to fork the addon and cut a release under a different name, so we can unblock the upgrade.

In the process of doing so, I also significantly refactored the code and "fixed" a few issues that were a problem for us previously.

The result was ember-class-based-modifier v0.9, which is released on NPM and ready for use in apps.

Here are a summary of the changes:

1. Renamed package.

   The "oo modifier" name always felt a little bit off to me, personally. First of all, we have a similar API which we call "class-based helpers" and so I would expect these to be called "class-based modifiers". It is also unnecessarily "jargony" – many Ember users may not know or think of themselves as "practicing OOP" while using Ember APIs. "OO" is also not a particularly notable thing, since the entire framework is built on top of that philosophy. 😄

   Perhaps a bit pedantic, but I also switched from the plural "modifiers" noun to the singular "modifier" noun in the package name. My reasoning was that this addon provides a single superclass for authoring your own modifier(s), as opposed to a collection of utility modifiers for you to use, so the package name should reflect that.

   So while it was a necessity for me to rename the package in my fork (since I don't have publish bit for this NPM package), I genuinely think "ember-class-based-modifier" is a better name anyway that I hope this addon can adopt.

   Picking a new name also allows us to make a few breaking changes to the API (see below) that would otherwise be tricky to deprecate.

2. Rewritten README.

   I rewrote the README, fixed a few bugs in the examples, refactored to apply correct Octane/classic idioms in the examples, removed some left overs from forking functional modifiers originally. I also decided to prioritize the "modern" ("native class") API and moved all classic examples to its own section.

   I also removed the reference to React's hooks API, since it's not useful for Ember users, and not a particularly good analogy anyway.

3. Removed the deprecated Modifier.modifier function.

   See Why is Modifier.modifier necessary? #7. It was already deprecated, and with this breaking release, I removed it entirely.

4. Made the manager a singleton.

   This is an implementation detail, but since the manager does not need the owner anymore, we could just have a single instance of it and avoid the WeakMap dance (there was also a bug in the current createManager code – the manager was never set on the WeakMap so the caching is not working as intended.

5. Used the factory for instantiation (classic API).

   Use factory.create instead of factory.class.create. This fixes some issues with injections. As an implementation detail, I also added a static create() on the "modern" ("native class") API so they can share the same code path.

6. Moved classic API to ember-class-based-modifier/classic.

   Personally, I find the previous import paths very confusing.

   Before:

   import Modifier from 'ember-oo-modifiers'; // Modern API
   import { Modifier } from 'ember-oo-modifiers'; // Classic API

   After:

   import Modifier from 'ember-class-based-modifier'; // Modern API
   import Modifier from 'ember-class-based-modifier/classic'; // Classic API

   When I was first reading the code and the README, I completely missed this difference (the extra { ... }) and it resulted in a lot of confusion.

7. Stopped install named arguments as properties.

   The current code tries to install the named arguments as properties on the modifier instance. That is, if you invoke {{some-modifier foo="bar"}}, then this.foo === 'bar' on the modifier instance.

   This is probably copied from Ember.Component, but it has a few problems:

   • First of all, it doesn't work. The current manager code only set these properties on create but never updates them.
   • Whatever your end user passes in will clobber your properties, e.g. {{some-modifier didReceiveArguments=this.lolwut willDestroy=this.staph}}.
   • There is no way to access positional arguments.

   This was generally viewed as a mistake of the Ember.Component API, something that we should not copy forward.

   Instead, I used the Glimmer component's design where arguments are available on this.args. Technically, it is the ember-control-flow-component design, since this gives you access to both named and positional arguments.

8. Arguments are not passed to hooks.

   This mirrors the life-cycle hooks in both classic and Glimmer components.

   Instead, we should encourage the user to create getters or CP aliases (in classic API) to access them off of this.args. See the examples in the rewritten README.

9. Renamed didInsertElement to didInstall and willDestroyElement to willRemove.

   The old names were a bit misleading.

   willDestroyElement imply that the element is being destroyed – if that's the case, you may think that it is safe to skip teardown work, since the element is going away anyway. However, there are cases where this hook maybe called when the element is going to stay. It really only implies that the modifier instance is going away.

   Similarly, didInsertElement may be called on on an element that is not actually "freshly inserted".

10. Corrected the life-cycle hooks ordering.

    I checked the current behavior against classic component's life-cycle hooks. The current order is different from the current ordering: didReceiveArguments should fire before didInsertElement (now didInstall), and didUpdateArguments should fire before didReceiveArguments.

    Since this addon will be an important tool for people migrating from classic components, we should match their order.

11. Swapped the native class constructor's arguments ordering.

    See Glimmer component unification #9. This is to match the Glimmer component API.

    This is also already a breaking change anyway, since the current manager code only passes the named arguments, and I changed it to pass both named and positional arguments (see item 7).

12. Added willDestroy, isDestroying and isDestroyed.

    This is to match both Ember.Object and Glimmer components. I copied to logic from Glimmer components' manager, to make sure the semantics are correct.

13. Rewrote the test suite.

    I also rewrote the test suite to remove the duplication between the tests for classic and modern API. Along the way, I've also improved the test coverage (e.g. adding tests for constructors, fully enumerate all the possible scenarios) and removed the tests related toModifier.modifier.

14. Added CI.

    It's now running here. 2.18 tests are failing due to Interop with EmberObject.extend on < 3.1 ember-polyfills/ember-modifier-manager-polyfill#11, but that should be fixed and released soon, at which point I will cut a 0.9.1 release.

My Proposal

While there were a lot of changes and refactors, I think ember-class-based-modifier is still fundamentally the same addon, based around the same ideas. This is going to be an important addon for the Octane migration, so I hope we can get it to the best possible shape before we open the flood gate!

If the changes I proposed are okay with you all, I would love to hand the keys back to the current maintainers. That would involve, at minimum, adding the current maintainers to the new NPM package, but we could probably merge the commits back into this repo and then rename the repo using the GitHub UI. That way, we can preserve all the history, issues, and URL (GitHub will redirect for you).

As far as the current addon, I suggest that we merge #13 and release 0.5.1. Maybe we can also include a deprecation warning suggesting current users to migrate to ember-class-based-modifier, but I'm not sure that's worth the noise.

Anyway, thank you for everyone's work in creating this addon, and I hope my changes will be useful!

[sukima]

I just merged #13, deprecated ember-oo-modifiers, and published a patched version: 0.5.2. @chancancode what is the next step?

[chancancode]

Thanks for the quick turnaround! I opened #15 with what I believe are the next steps!