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

[BUGFIX beta] Ensures Ember.A is a constructor #17238

Merged

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Nov 29, 2018

The recent change to transpilation has made new A() break, because
A is an arrow function and arrow functions cannot be constructors. This
is a pretty common pattern in Ember apps and likely constitutes a
breaking change, so this PR fixes it by changing the arrow function to
a normal function.

@pzuraq
Copy link
Contributor Author

pzuraq commented Nov 29, 2018

cc @rwjblue

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

I don’t think we intended new Ember.A() (I’ve generally always used it without new), but it seems fine to fix if recent changes broke it.

Can you also add a test?

@rwjblue
Copy link
Member

rwjblue commented Nov 29, 2018

Lets add a private API deprecation (until: ‘3.9.0') for the new case actually, along with the test....

@pzuraq
Copy link
Contributor Author

pzuraq commented Nov 29, 2018

Updated, and added PR to the deprecation app

@rwjblue
Copy link
Member

rwjblue commented Nov 29, 2018

FWIW, there is a bit of usage of new A( / new Ember.A out in the wild, definitely justifies at least a private API deprecation like this:

https://emberobserver.com/code-search?codeQuery=new%20A(
https://emberobserver.com/code-search?codeQuery=new%20Ember.A(

@pzuraq pzuraq force-pushed the bugfix/ensure-array-wrapper-is-a-constructor branch from b6cd574 to 488195d Compare November 29, 2018 21:16
{
id: 'array.new-array-wrapper',
until: '3.9.0',
url: 'https://emberjs.com/deprecations/v3.x#toc_new-array-wrapper',
Copy link
Member

Choose a reason for hiding this comment

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

Seems to be wrong (the url doesn't link directly), when I find the right link it looks like:

https://deprecations-app-prod.herokuapp.com/deprecations/v3.x/#toc_array-new-array-wrapper

Can you confirm which is correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are correct! Updated

The recent change to transpilation has made `new A()` break, because
A is an arrow function and arrow functions cannot be constructors. This
is a pretty common pattern in Ember apps and likely constitutes a
breaking change, so this PR fixes it by changing the arrow function to
a normal function, and deprecates this behavior.
@pzuraq pzuraq force-pushed the bugfix/ensure-array-wrapper-is-a-constructor branch from 488195d to 5a565d5 Compare November 29, 2018 21:36
@rwjblue rwjblue merged commit e329e3f into emberjs:master Nov 30, 2018

['@test new Ember.A'](assert) {
expectDeprecation(() => {
assert.deepEqual(new A([1, 2]), [1, 2], 'array values were not be modified');
Copy link
Member

Choose a reason for hiding this comment

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

ya, woah this was never an intended feature...

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.

None yet

3 participants