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

Deprecate Ember.merge #16956

Merged
merged 4 commits into from
Sep 10, 2018
Merged

Deprecate Ember.merge #16956

merged 4 commits into from
Sep 10, 2018

Conversation

RobbieTheWagner
Copy link
Member

No description provided.

let eventOpts = merge({}, DEFAULT_EVENT_OPTIONS);
merge(eventOpts, options);
let eventOpts = assign({}, DEFAULT_EVENT_OPTIONS);
assign(eventOpts, options);
Copy link
Member

Choose a reason for hiding this comment

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

Can you replace this with a single assign?

let eventOpts = merge({}, DEFAULT_EVENT_OPTIONS);
merge(eventOpts, options);
let eventOpts = assign({}, DEFAULT_EVENT_OPTIONS);
assign(eventOpts, options);
Copy link
Member

Choose a reason for hiding this comment

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

Same here? We should only need one assign...

deprecate('Use of `merge` has been deprecated. Please use `assign` instead.', false, {
id: 'ember-polyfills.deprecate-merge',
until: '4.0.0',
url: 'https://emberjs.com/deprecations/v3.x/#toc_ember-polyfills-deprecate-merge',
Copy link
Member

Choose a reason for hiding this comment

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

Is this created yet? Can you link me to the deprecation app PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not created yet. I need to look into what I need to do for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@rwjblue
Copy link
Member

rwjblue commented Sep 9, 2018

We also need to guard this with deprecation flags for svelting, something like #16745.

@RobbieTheWagner
Copy link
Member Author

@rwjblue I added a link to the deprecations app PR and attempted the svelting stuff. Not sure I did it exactly right, so feel free to let me know what I should tweak. One thing I wasn't sure on is what version we should mark this deprecation as introduced in? I randomly chose 3.5 for both the svelting here and the deprecations app PR, but I think it will have to be a later version probably, since I think 3.5 beta has already been cut?

@rwjblue
Copy link
Member

rwjblue commented Sep 9, 2018

Correct, master currently represents 3.6. We could backport the deprecation into 3.5 betas but I don’t see why we would necessarily want to do that so let’s just say 3.6...

@RobbieTheWagner
Copy link
Member Author

@rwjblue just updated to point to 3.6. Let me know if there are any other tweaks I should make 😄

@rwjblue
Copy link
Member

rwjblue commented Sep 10, 2018

I merged the deprecation PR, it should redeploy soon, would you mind confirming the url here works properly?

Once we’ve done that we can merge...

@locks
Copy link
Contributor

locks commented Sep 10, 2018

https://emberjs.com/deprecations/v3.x/#toc_ember-polyfills-deprecate-merge is up and working :)

@RobbieTheWagner
Copy link
Member Author

@rwjblue looks like it's deployed and working. Anything else we need to tweak here?

@rwjblue rwjblue merged commit a07ad1b into master Sep 10, 2018
@rwjblue rwjblue deleted the deprecate-ember-merge branch September 10, 2018 12:47
@kennethlarsen kennethlarsen mentioned this pull request Nov 7, 2018
3 tasks
@rwjblue rwjblue mentioned this pull request Jan 23, 2019
16 tasks
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