Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate computed().meta() #441

Closed
wants to merge 2 commits into from

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Feb 7, 2019

@vitch
Copy link

vitch commented Feb 14, 2019

We do make use of computed().meta() and metaForProperty() in our codebase. Looking for docs on that I notice it's marked as private anyway and not documented from 2.16 onwards (although it's definitely working without deprecation warnings in 2.18).

Our use case is from a macro function that returns a CP. We add some metadata to the CP which we use in tests in order to verify that the property was passed the expected configuration.

I guess we could fairly easily switch to a different way of doing this - and we probably will need to as we convert to ES6 classes / decorators anyway. I just wanted to call out that the functionality is being used in more places than a search of open source code would have returned...

@pzuraq
Copy link
Contributor Author

pzuraq commented Feb 14, 2019

Hey @vitch! That's great to hear from you as a user of the API, I absolutely understand that it's not possible search all Ember code and that there may be users in private code that I don't have visibility into.

The searches through emberobserver.com are generally used as barometer, because usually usage is fairly similar in private and public spaces - for instance, Ember.meta is technically a private API, but it's used commonly in addons and that's a good sign that it may be used in many apps as well. This is why it has been considered "intimate API" for some time. This is definitely not always the case - sometimes something is used commonly in apps and not very commonly in addons, or vice-versa, so hearing from actual experiences of app devs is very useful!

For the RFC itself, I'll clarify that there will be a deprecation guide for any existing users. In the documentation section, I was mainly trying to stay away from documenting the usage of private APIs like eachComputedProperty, since those are tied to lots of functionality in Ember meta that we probably don't want to keep supporting in the future, and want to migrate users like Ember Data off of.

@rwjblue rwjblue added the T-framework RFCs that impact the ember.js library label Feb 14, 2019
@rwjblue rwjblue self-assigned this Feb 14, 2019
@igorT igorT requested review from igorT and runspired February 23, 2019 00:09
@ming-codes
Copy link

I need some clarification on this. Is this trying to remove the ability to attach meta properties to computed property or just trying to change the way it is exposed?

@pzuraq
Copy link
Contributor Author

pzuraq commented Feb 26, 2019

We are trying to remove the ability to do it directly. Instead, the recommended path forward would be to use a WeakMap to attach the meta information:

const META = new WeakMap();

function myComputedMacro() {
  let cp = computed(...);

  META.set(cp, { /* meta information here */ };
}

// later on

let cp = myComputedMacro();

META.get(cp); // this is how you access your meta information.

This way, storing meta information would be entirely user code, without any framework code. This is the same pattern that will likely be used in the future with native decorators, in JavaScript apps in general, not just Ember.

We are also talking about changing private APIs, such as eachComputedProperty. This API has never been marked as public, and is a way to iterate over the meta values for every computed property on the class. We would change it to iterate over the computed properties themselves, so user code could access their own WeakMap with the cp instances, like described above. This would be a breaking change, but it is private API and we haven't found much usage of it in the wild. If you do use this API, please let us know!

@runspired
Copy link
Contributor

👍 all for this. We do make use of this in EmberData but it's something we can refactor away trivially and I've been meaning to do so.

@runspired
Copy link
Contributor

While not directly related to removing meta (we could do so without removing use of computed), I did find that removing the use of computed from EmberData required multiple intimate APIs and ultimately hit a blocker in which set always reads the value before write unless you are a computed (which would trigger expensive calculations that are undesirable even in the first read case) emberjs/data#7477

Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a path forward here for EmberData without computed.meta. I would note though that one thing that this RFC does not address is how metaForProperty and eachComputedProperty will ensure proto() has been called first to build the inheritance chain and walk in a way such that you won't encounter duplicate props from inherited mixins and classes. This is deduping and chain walking is something that custom meta code would have to do as well.

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

@runspired how's Ember Data looking on this now?

@runspired
Copy link
Contributor

We cannot remove our use of computed and computed meta-without first eliminating our support for classic

@wagenet
Copy link
Member

wagenet commented Jul 29, 2022

I believe this falls under the umbrella of #832. I'm going to close this issue to push any discussion there. Despite being closed, we will certainly use this issue for reference and to inform the future RFC. Thank you all for the discussion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-framework RFCs that impact the ember.js library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants