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.

Sign up for GitHub

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

Jump to bottom

[Fixes #15001] Remove internal EmptyObject usage #15002

Merged
merged 1 commit into from
Mar 14, 2017
Merged

[Fixes #15001] Remove internal EmptyObject usage #15002

merged 1 commit into from
Mar 14, 2017

Conversation

stefanpenner
Copy link
Member

@stefanpenner stefanpenner commented Mar 11, 2017 •
edited
Loading

  • confirm all are correctly removed
  • confirm tests pass
  • do some tests

If this ends up good, we should likely consider deprecating Ember.EmptyObject.

cc @bmeurer

@stefanpenner stefanpenner mentioned this pull request Mar 11, 2017
Closed
rwjblue
rwjblue reviewed Mar 12, 2017
View reviewed changes
packages/ember-utils/lib/empty-object.js Outdated
// when you're treating the object instances as arbitrary dictionaries
// and don't want your keys colliding with build-in methods on the
// default object prototype.
// This exists because `Object.create(null)` WAS slow compared to `new
Copy link
Member

Choose a reason for hiding this comment

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

We should likely remove this file completely. I did an ember-observer code search (here) and there are no direct usages of our version any longer (though there are addons that have made their own copies of this code).

Copy link
Member Author

Choose a reason for hiding this comment

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

i thought ember-data used it, but it ships its own: https://github.com/emberjs/data/search?utf8=%E2%9C%93&q=EmptyObject&type=Code

Which we should then also remove

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I had already created an issue over there for removing it too 😸

Copy link
Member Author

Choose a reason for hiding this comment

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

emberjs/data#4853

@stefanpenner stefanpenner force-pushed the empty-object branch 2 times, most recently from 7f5190a to 48410cb Compare March 12, 2017 07:39
@stefanpenner
Copy link
Member Author

ember-data PR: emberjs/data#4854

@stefanpenner stefanpenner mentioned this pull request Mar 12, 2017
Merged
@homu
Copy link
Contributor

homu commented Mar 14, 2017

☔ The latest upstream changes (presumably #15006) made this pull request unmergeable. Please resolve the merge conflicts.

@stefanpenner
Copy link
Member Author

@homu r+

@homu
Copy link
Contributor

homu commented Mar 14, 2017

📌 Commit c06d82d has been approved by stefanpenner

@homu
Copy link
Contributor

homu commented Mar 14, 2017

⚡ Test exempted - status

@homu homu merged commit c06d82d into master Mar 14, 2017
homu added a commit that referenced this pull request Mar 14, 2017
@homu
Auto merge of #15002 - emberjs:empty-object, r=stefanpenner
1a8791f 
[Fixes #15001] Remove internal EmptyObject usage

- [x] confirm all are correctly removed
- [x] confirm tests pass
- [ ] do some tests

----

If this ends up good, we should likely consider deprecating `Ember.EmptyObject`.

---

cc @bmeurer
@homu homu mentioned this pull request Mar 14, 2017
Merged
@stefanpenner stefanpenner deleted the empty-object branch March 14, 2017 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stefanpenner @homu @rwjblue