Map and MapWithDefault marked as private

[Turbo87]

The Map and MapWithDefault classes are currently marked as @private, yet they are exported through the shims. It looks like the @private tag was added in #11362.

It seems inconsistent to me to export the classes but mark them as private so I'd like to propose marking them as public.

/cc @pangratz

[rwjblue]

The shims aren't guaranteed to all point to public things.

I'm unsure if we intend to have these be public or not. I'd like to hear from other core team members...

[Turbo87]

I'd like to hear from other core team members...

can I interpret the 👍 from @stefanpenner as acknowledgement? 😉

[Turbo87]

@rwjblue @stefanpenner any news on this?

[stefanpenner]

because ember-data uses them, they are at the very least intimate API. I would be fine to make them public, as Map is a pretty close to a strict subset of ES6 Map. So if we switch to ES6 map, it should "just work". We should confirm that MapWithDefault could be implemented as a subclass. If those are both true, I would push for making them public.

[pixelhandler]

@stefanpenner @rwjblue MapWithDefault https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/map.js#L412-L446 extends Map so when it becomes the ES6 Map instead, using Object.create(Map.prototype); should still work for subclassing, correct?

[krisselden]

I'm strongly opposed to this, this should not be in core, they are not used by anything in core and currently only used in Ember Data. This is the kind of thing we need to drop to lighten up core.

[rwjblue]

Seems good to me. We should check in with the ember-data folks about a migration path...

[stefanpenner]

I'm strongly opposed to this, this should not be in core, they are not used by anything in core and currently only used in Ember Data. This is the kind of thing we need to drop to lighten up core.

This is used in many add-ons and apps, regardless how we feel this is in core, it is defacto public. Marking it as private, and removing it wont yield much for apps as nearly everyone will pull it back in immediately, which will result in a pointless deprecation. We should be honest, make it public and as part of a future tree-shaking (or ES6 friendly) effort re-visit.

[Turbo87]

@rwjblue @stefanpenner has there been any discussion on this since last year? if the classes are not used by Ember itself IMHO we should remove them. we could build a sort of shim addon that checks the Ember version at buildtime and if Ember provides the classes ifself it would delegate to Ember, and otherwise provide the implementation itself (or even delegate to the native ES6 Map).

[rwjblue]

The basic plan forward here is:

• Create separate addons for:
  • ember-ordered-set - This would contain the implementation from here in its index.js.
  • ember-map-polyfill - This would:
    • do nothing if all targets support native Map
    • include a Map polyfill (likely just bring over the implementation here in emberjs/ember.js)
  • ember-map-with-default - Depends on ember-map-polyfill and extends native Map
• Update ember-data to use ember-map-with-default and ember-ordered-set (as needed).
• Once ember-data changes land in release version, land deprecations from [DEPRECATION] Remove ember-metal/map #15119 (with until: '3.5.0') .
• profit 💰

[Turbo87]

@rwjblue thanks for the detailed plan. I'll put it on the todo list :)

[locks]

Repos up:

• https://github.com/emberjs/ember-ordered-set
• https://github.com/emberjs/ember-map-polyfill
• https://github.com/emberjs/ember-map-with-default

[pixelhandler]

@rwjblue @locks Since the repos are up I'll close this for now.