Skip to content

[BUGFIX] observed properties not being marked as enum#16735

Merged
rwjblue merged 1 commit intoemberjs:masterfrom
bekzod:fix-object-key
Jan 4, 2019
Merged

[BUGFIX] observed properties not being marked as enum#16735
rwjblue merged 1 commit intoemberjs:masterfrom
bekzod:fix-object-key

Conversation

@bekzod
Copy link
Copy Markdown
Contributor

@bekzod bekzod commented Jun 10, 2018

fixes #14594

@bekzod bekzod force-pushed the fix-object-key branch 2 times, most recently from b5e6530 to 2cbd94d Compare June 10, 2018 13:04
Copy link
Copy Markdown
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.

Fix looks good to me, only a few tweaks needed to the test then we can land...

getOwner(proxy);
}

['@test `Object.keys` works properly'](assert) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you tweak this test title? I'd like to see it reference the original issue, and specifically what it is intended to test.

Something like this seems reasonable:

@test observed properties are enumerable when set GH#14594

Also, can you move this to two tests one in packages/ember-metal/tests/accessors/set_test.js and another in packages/ember-metal/tests/set_properties_test.js?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sure

@bekzod bekzod force-pushed the fix-object-key branch 2 times, most recently from 8fbf9c9 to 04c48cb Compare June 11, 2018 15:09
@@ -1,4 +1,5 @@
import { setProperties } from '..';
import { CoreObject } from 'ember-runtime';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In general we shouldn't be importing from ember-runtime from within ember-metal.

The tests in ember-metal will need to be updated to use POJO's instead of CoreObject...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Issue does not happen with POJO objects it only happens with CoreObject, so moving back to CoreObject test

@bekzod bekzod force-pushed the fix-object-key branch 2 times, most recently from 02bcae9 to f3eca1c Compare September 18, 2018 08:19
@Turbo87 Turbo87 requested a review from rwjblue December 19, 2018 16:53
@rwjblue rwjblue merged commit a97afa8 into emberjs:master Jan 4, 2019
@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Jan 4, 2019

Sorry it took so long here, but this looks good to me. Thanks again @bekzod!

@bekzod bekzod deleted the fix-object-key branch January 8, 2019 07:34
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.

Ember.set & Ember.setProperties fail to set properties "enumerable"

2 participants