Declare Ember.defineProperty as public?

[simonihmig]

I am opening this here for discussion, as I have seen confusing statements (to me at least) in other repos/issues, so would like to clarify this here in a central place...

Problem

Ember.defineProperty is declared private, although it seems to be widely used in the wild (see below). IMHO there is at least one valid use case for it, when the dependent keys of a CP are not known it advance.

Status quo

• used in addons where the dependent keys are not known in advance. Examples:
  • https://github.com/offirgolan/ember-bootstrap-cp-validations/blob/7a85bf27796d2ac62a57d5b6a236c888fc102916/addon/components/bs-form/element.js#L23 (cc @offirgolan)
  • emberobserver.com finds 201 addons containing defineProperty. Admittedly that also includes usage of Object.defineProperty, but a quick scan suggested a significant use of Ember.defineProperty as well!
• mentioned in blogs/other media. Examples:
  • http://mcdowall.info/an-experiment-in-refactoring-ember-properties/
  • https://stackoverflow.com/questions/38970530/creating-alias-to-a-dynamic-property-now-that-ember-binding-is-deprecated
• mentioned in an official deprecation guide: https://deprecations-app-prod.herokuapp.com/deprecations/v3.x/#toc_ember-meta-descriptor-on-object (which kicked off this discussion I believe, see Missing docs for defineProperty ember.js#16790)
• named export in RFC176 data: https://github.com/ember-cli/ember-rfc176-data (which only makes sense when considered public, right?)

Opinions

• @ef4 suggested on Slack recently, that it should remain private. The relevant transcript can be found in this issue: Clarify use of private API for deprecation solution ember-learn/deprecation-app#157
• @rwjblue's comment suggests making it public (if I read it correctly)
• my own personal take: the use case of declaring CPs with dynamic dependent keys would justify making this public (FWIW, I always considered it public, without checking 🤨). The solution offered by @ef4 (see link above, third bullet point) seems pretty inconvenient for this IMHO valid use case.

/cc @jenweber @ef4 @rwjblue

[rwjblue]

I generally agree with Ed’s points (over in the deprecation app issue), but frankly they do not cover all of the cases where Ember.defineProperty is used. I totally agree that most usages of Ember.defineProperty should be replaced (the polyfill use case, setting CPs on POJOs, etc) with more idiomatic scenarios.

However, Ember.defineProperty is the only way to properly setup a computed property on an ES class (regardless of dynamic dependent keys). “Properly” here means that it sets up the prototypes meta, installs native getters (in 3.1+), sets up “mandatory setter” in development mode, etc. Tooling like ember-decorators therefore must use Ember.defineProperty and it should be marked as public (it definitely meets the bar for intimate API already).

[simonihmig]

it should be marked as public

would that require a full-blown RFC process?

[rwjblue]

IMHO, no (it would just require a PR to change the docs from @private to @public), but it is something that we (the core team) should generally agree on before doing.

I'll try to take this to the next core team meeting to confirm we have consensus...

[ef4]

I defer to @rwjblue on this.

In general it would be nice to flag APIs like this as “advanced” so that newbies don’t encounter them in the normal docs.

[buzzware]

While the wind seems to be blowing towards static/explicit typing, I hope that Ember continues to provide "sharp knives" like this to enable meta-programming and other dynamic approaches.