-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[BUGFIX beta] Use native Map if present. #5255
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
My only concern is that we're dropping functions from the return type of public API.
Getting people off the "polyfill+" is definitely good, but maybe we can provide (at least in dev mode) a more useful error message than "undefined is not a function".
``` | ||
*/ | ||
|
||
export default class MapWithDefault { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add copy
and isEmpty
to warn/error with a useful message for people who may be relying on this?
It seems unlikely, but on the other hand, MapWithDefault
is returned by some public API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'm going to try and pick this back up today/tomorrow to cleanup and address the points @hjdivad mentioned above... |
When native `Map` is present, use it. Otherwise fallback to `Ember.Map`. This positions ember-data for the eventual deprecation and dropping of `Ember.Map` and `Ember.MapWithDefault` in Ember itself (and using native classes should generally be better for us long term than shipping custom _slightly_ different versions). Note there are a few specific differences between native `Map` (and the `MapWithDefault` implementation that extends from the native `Map` when possible) and Ember's `Ember.Map` that we should be aware of: * `Ember.Map` has custom `copy` and `isEmpty` methods which are not present in native `Map` * `Ember.Map` adds a static `create` method (which simply instantiates itself with `new Ember.Map()`) * `Ember.Map` does not accept constructor arguments * `Ember.Map` does not have: * `@@species` * `@@iterator` * `entries` * `values`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @rwjblue. |
When native `Map` is present, use it. Otherwise fallback to `Ember.Map`. This positions ember-data for the eventual deprecation and dropping of `Ember.Map` and `Ember.MapWithDefault` in Ember itself (and using native classes should generally be better for us long term than shipping custom _slightly_ different versions). Note there are a few specific differences between native `Map` (and the `MapWithDefault` implementation that extends from the native `Map` when possible) and Ember's `Ember.Map` that we should be aware of: * `Ember.Map` has custom `copy` and `isEmpty` methods which are not present in native `Map` * `Ember.Map` adds a static `create` method (which simply instantiates itself with `new Ember.Map()`) * `Ember.Map` does not accept constructor arguments * `Ember.Map` does not have: * `@@species` * `@@iterator` * `entries` * `values` (cherry picked from commit f60d1e9)
When native
Map
is present, use it. Otherwise fallback toEmber.Map
.This positions ember-data for the eventual deprecation and dropping of
Ember.Map
andEmber.MapWithDefault
in Ember itself (and using native classes should generally be better for us long term than shipping custom slightly different versions).Note there are a few specific differences between native
Map
(and theMapWithDefault
implementation that extends from the nativeMap
when possible) and Ember'sEmber.Map
that we should be aware of:Ember.Map
has customcopy
andisEmpty
methods which are not present in nativeMap
Ember.Map
adds a staticcreate
method (which simply instantiates itself withnew Ember.Map()
)Ember.Map
does not accept constructor argumentsEmber.Map
does not have:@@species
@@iterator
entries
values
Addresses
Ember.Map
andEmber.MapWithDefault
usages from #5254.