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

[FEATURE ember-metal-ember-assign] Introduce Ember.assign() #12303

Merged
merged 1 commit into from
Sep 14, 2015

Conversation

tricknotes
Copy link
Member

Ember.assign() is discussed in #11978 .

  • Add feature flagging for the API addition
  • Add a test for the deprecation of Ember.merge when the flag is enabled
  • Add some cursory tests for ember-metal/assign (doesn't need to be complicated, but I noticed no tests were tweaked/changed for merge or assign and figured we probably want some to exist).
  • We should use Object.assign if it exists (making our Ember.assign a straight up polyfill).

@@ -321,6 +322,7 @@ Ember.isEmpty = isEmpty;
Ember.isBlank = isBlank;
Ember.isPresent = isPresent;

Ember.assign = assign;
Copy link
Member

Choose a reason for hiding this comment

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

I think we will need to feature flag the public API change.

@rwjblue
Copy link
Member

rwjblue commented Sep 8, 2015

Could you:

  • Add feature flagging for the API addition
  • Add a test for the deprecation of Ember.merge when the flag is enabled
  • Add some cursory tests for ember-metal/assign (doesn't need to be complicated, but I noticed no tests were tweaked/changed for merge or assign and figured we probably want some to exist).

@rwjblue
Copy link
Member

rwjblue commented Sep 10, 2015

Another point made in #12320 (that I agree with) is that we should use Object.assign if it exists (making our Ember.assign a straight up polyfill).

@rwjblue
Copy link
Member

rwjblue commented Sep 10, 2015

@tricknotes - I moved the various comments/todo items into a checklist in the PR description (so you can check them off as you go).

@tricknotes
Copy link
Member Author

Sorry for my late :<
Thanks @rwjblue . Yes, I'm agree with these todo checklist!

I'll continue to work in this weekend.

@oriSomething
Copy link

if there is a well known and widely use of a polyfill, why writing a code for it and not using it?

@tricknotes
Copy link
Member Author

if there is a well known and widely use of a polyfill, why writing a code for it and not using it?

I think Ember.assign is enough to work just merge some objects. It might not required to be strict as Object.assign.

@tricknotes
Copy link
Member Author

@rwjblue
All todo things are done.
Could you review again?

@rwjblue
Copy link
Member

rwjblue commented Sep 13, 2015

@tricknotes - Looks great, can you squash commits and make the new commit's title [FEATURE ember-metal-ember-assign]?

@tricknotes tricknotes changed the title Introduce Ember.assign() [FEATURE ember-metal-ember-assign] Introduce Ember.assign() Sep 14, 2015
@tricknotes
Copy link
Member Author

Thanks, @rwjblue .
done.

@rwjblue
Copy link
Member

rwjblue commented Sep 14, 2015

Awesome work as usual, thank you @tricknotes!

rwjblue added a commit that referenced this pull request Sep 14, 2015
[FEATURE ember-metal-ember-assign] Introduce Ember.assign()
@rwjblue rwjblue merged commit 000a248 into emberjs:master Sep 14, 2015
@tricknotes tricknotes deleted the ember-assign branch September 14, 2015 02:22
@oriSomething
Copy link

@tricknotes @rwjblue i think if Ember.assign is supposed to be a polyfill when there is no Object.assign, it suppose to support also copying of Symbols as Object.assign does.

There are versions of chrome that support Symbol but no Object.assign, so need is exist.

@rwjblue
Copy link
Member

rwjblue commented Sep 14, 2015

We are not intending to make a full fidelity polyfill here, just the behaviors that we need. If you need/want a full polyfill you should include a full fidelity version (like the one you linked) and that will be used instead (since it actually sets Object.assign).

@oriSomething
Copy link

@rwjblue but three things:

  • it might lead to confusion. there are still browsers that support Symbols and not Object.assign if it's not a polyfill, why naming as one and more over use Object.assign when available? better left with merge and prevent confusion
  • it will lead to unexpected behavior, maybe is seems like very edge case because Symbols are not in a wide usage, be it still might cause a bugs and unexpected behaviors in some browsers. specially in future where more people will use them, and more mobile phone with various mobile browsers
  • it's only a few lines of code to add to support Symbols and make it a full polyfill. much shorter than this comment

i know it sounds picky and very edge case, but as web developers we all fight very edge cases everyday. and many don't know enough Ember nor Object.assign and we might cause them many trouble for so little code. in the bottom we better not use it as a polyfill or fully polyfill this feature

homu added a commit to fenichelar/ember-simple-auth-token that referenced this pull request Mar 30, 2016
fix deprecation Ember.merge for 2.5.0

DEPRECATION: Usage of `Ember.merge` is deprecated, use `Ember.assign` instead. [deprecation id: ember-metal.merge]

More info: emberjs/ember.js#12303

Setup:

```
Ember             : 2.5.0-beta.3
Ember Data        : 2.5.0-beta.3
jQuery            : 2.2.1
Ember Simple Auth : 1.1.0-beta.3
```
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