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] observed properties not being marked as enum #16735

Merged
merged 1 commit into from Jan 4, 2019

Conversation

Projects
None yet
2 participants
@bekzod
Copy link
Contributor

bekzod commented Jun 10, 2018

fixes #14594

@bekzod bekzod force-pushed the bekzod:fix-object-key branch 2 times, most recently from b5e6530 to 2cbd94d Jun 10, 2018

@rwjblue
Copy link
Member

rwjblue left a comment

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

@@ -93,6 +93,28 @@ moduleFor(
getOwner(proxy);
}

['@test `Object.keys` works properly'](assert) {

This comment has been minimized.

Copy link
@rwjblue

rwjblue Jun 11, 2018

Member

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?

This comment has been minimized.

Copy link
@bekzod

bekzod Jun 11, 2018

Author Contributor

sure

@bekzod bekzod force-pushed the bekzod:fix-object-key branch 2 times, most recently from 8fbf9c9 to 04c48cb Jun 11, 2018

@@ -1,4 +1,5 @@
import { setProperties } from '..';
import { CoreObject } from 'ember-runtime';

This comment has been minimized.

Copy link
@rwjblue

rwjblue Jun 12, 2018

Member

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...

This comment has been minimized.

Copy link
@bekzod

bekzod Jul 2, 2018

Author Contributor

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

@bekzod bekzod force-pushed the bekzod:fix-object-key branch from 04c48cb to 799c72c Jul 2, 2018

@bekzod bekzod force-pushed the bekzod:fix-object-key branch 2 times, most recently from 02bcae9 to f3eca1c Sep 17, 2018

@Turbo87 Turbo87 requested a review from rwjblue Dec 19, 2018

@bekzod bekzod force-pushed the bekzod:fix-object-key branch from f3eca1c to 905f6db Dec 24, 2018

@bekzod bekzod force-pushed the bekzod:fix-object-key branch from 905f6db to 79494e2 Dec 24, 2018

@rwjblue

rwjblue approved these changes Jan 4, 2019

@rwjblue rwjblue merged commit a97afa8 into emberjs:master Jan 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rwjblue

This comment has been minimized.

Copy link
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 bekzod:fix-object-key branch Jan 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.