Skip to content

WeakMap RFC#91

Merged
mmun merged 3 commits intomasterfrom
weakmap
Jun 14, 2016
Merged

WeakMap RFC#91
mmun merged 3 commits intomasterfrom
weakmap

Conversation

@stefanpenner
Copy link
Member

@mixonic
Copy link
Member

mixonic commented Sep 12, 2015

Generally, very very +1. Awesome.

@stefanpenner This suggests that the Ember WeakMap would not support map -> key weakness. Correct?

If so, destroy on the weak map would still remove the data for all current members from memory?

@stefanpenner
Copy link
Member Author

Destroying the weakmap itself would have no affect. The data would just become inaccessible and require the keys be reclaimed before

@stefanpenner
Copy link
Member Author

In this case the WM is basically just a way to store private state on an object without polluting or causing shape issues. In essence opening meta in a safe way as a baking store to userland. Primarily targeted at pro user addons features

@mixonic
Copy link
Member

mixonic commented Sep 12, 2015

"Motivation" should likely include the specific Ember features this would be used for. To quote Stef via chat:

  • Fixing computed sort nicely
  • Potential future feature of "runtime dependent key" for cp

There are some addons that would use this:

  • ember-state-service
  • ember-cp-validations

Though those could also consume WeakMap as a stand-alone library instead. The stand-alone version would be dependent on Ember meta however, which would definitely be reaching into something private. Seems like it should be mentioned under "Alternatives" (not that I'm advocating for this alternative).

edit: @ef4 reminds me this is not actually a viable alternative, as the meta is created on demand and thus not practical to use from outside Ember. This is a good motivation to keep this feature as part of Ember.

@stefanpenner
Copy link
Member Author

Actually it isn't that meta is created on demand that is the problem. We can trigger its creation from userland. The issue is more of shaping. We can't ensure meta shapes correctly with this new slot. Unless we can insert the slot in its constructor.

In theory run times should be able to adapt to afew dynamic slots added. In reality, we blow our budget of such flexibility easily.

We will publish a polyfil addon, so older code can use it. Unfortunately that addon will be stuck causing meta to have different slots

@jasonmit
Copy link
Member

jasonmit commented Mar 6, 2016

@stefanpenner based on the WeakMap spec, delete returns a boolean on whether or not the delete found anything. In your proposal, it returns an instance of the weakmap. Just curious if intentional or does the RFC need to be updated.

Related: thoov/ember-weakmap#3

@stefanpenner
Copy link
Member Author

we should follow the spec https://tc39.github.io/ecma262/#sec-weakmap.prototype.delete so returning true/false

@stefanpenner
Copy link
Member Author

@stefanpenner
Copy link
Member Author

This RFC is still required to eventually make it public, as i believe the built-in weakmap is still private.

@mmun
Copy link
Member

mmun commented Jun 14, 2016

Looks good 👍

@mmun mmun merged commit 5e8001b into master Jun 14, 2016
@mmun mmun deleted the weakmap branch June 14, 2016 20:20
@stefanpenner
Copy link
Member Author

next step: emberjs/ember.js#13668

kategengler pushed a commit that referenced this pull request Jan 19, 2019
…tal-hooks

Promote buildInstrumentation to public API
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants