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

Remove custom implementations of Map/MapWithDefault/OrderedSet #15898

Merged
merged 1 commit into from
Dec 19, 2018
Merged

Remove custom implementations of Map/MapWithDefault/OrderedSet #15898

merged 1 commit into from
Dec 19, 2018

Conversation

maxscott
Copy link
Contributor

#13815

MapWithDefault's usage in ember-data is accounted for here (emberjs/data/pull/5255)
OrderedSet should be installed as an addon by any project using it.

This keeps the vendor shim's 'ember-map'.default = Ember.Map, which is now native Map. Unsure if it's better to remove the alias/shim altogether.

@Turbo87
Copy link
Member

Turbo87 commented Dec 1, 2017

Unsure if it's better to remove the alias/shim altogether.

IMHO it should be removed completely

@rwjblue why are there shims in this repo at all? I thought only ember-cli-shims provided them?! 🤔

@rwjblue
Copy link
Member

rwjblue commented Dec 1, 2017

@rwjblue why are there shims in this repo at all? I thought only ember-cli-shims provided them?! 🤔

They are not used here. We noop'ed them just before initial npm release (by simply not app.import'ing them) in #14808, but never came back to clean up. They should definitely be removed, but in a separate PR...

@rwjblue
Copy link
Member

rwjblue commented Dec 1, 2017

I submitted #15909 to remove.

@Turbo87
Copy link
Member

Turbo87 commented Dec 4, 2017

@maxwerr can you rebase?

@maxscott
Copy link
Contributor Author

maxscott commented Dec 4, 2017

@Turbo87 rebased. shims.js removed

@maxscott
Copy link
Contributor Author

I figured today would be a good time to delete a bunch of private, seemingly unused code before someone else does! Lmk if this requires anything else I can do.

Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

I've rebased this PR. CI is green. v3.5.0 has been released a long time ago. I'd say this is ready to be merged.

/cc @rwjblue

@rwjblue rwjblue merged commit 25c9945 into emberjs:master Dec 19, 2018
@rwjblue
Copy link
Member

rwjblue commented Dec 19, 2018

Thanks @maxscott and @Turbo87!

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.

3 participants