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

[CHORE] RFC-0329 - adding deprecation about using evented in ember-data #6059

Merged
merged 6 commits into from
Jul 3, 2019

Conversation

pete-the-pete
Copy link
Contributor

Deprecating Evented for various ember-data classes.

@runspired runspired self-assigned this Apr 24, 2019
@runspired runspired added the code-review Tracks PRs that require a code-review label Apr 24, 2019
Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

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

Thanks!

This seems pretty close, I suspect if we fix the method name it'll mostly come down to figuring out how to resolve these deprecations in our own code.

@pete-the-pete
Copy link
Contributor Author

@runspired updated deprecatedTest but just using a barebones comparison function. I saw that we are using semver, but that's only available in node...and I didn't know if it would be worth adding another dependency. I think I got all the relevant tests, but I'll take another look tomorrow.

@pete-the-pete pete-the-pete force-pushed the RFC-0329 branch 4 times, most recently from b9151ab to 3df4ba1 Compare May 14, 2019 15:56
@pete-the-pete
Copy link
Contributor Author

🤦‍♂ finally realized that this is failing production tests...looking into that now. Also 👍 to having tests that run on a production build.

@pete-the-pete pete-the-pete force-pushed the RFC-0329 branch 3 times, most recently from 14614df to 4494572 Compare May 15, 2019 05:13
@pete-the-pete
Copy link
Contributor Author

@hjdivad I keep getting a failed test with ember-m3:
not ok 140 Chrome 74.0 - [23 ms] - unit/model: .save errors getting updated via the store and removed upon setting a new value

Running the tests locally with yarn test-external:ember-m3 I don't get any test failures, any ideas?

@hjdivad
Copy link
Member

hjdivad commented May 15, 2019

@pete-the-pete have you tried clearing out ../__external-test-cache and ../__tarball-cache?

FWIW I do get failures when i yarn test-external:ember-m3 after `node ./bin/packages-for-commit.js'

@pete-the-pete
Copy link
Contributor Author

pete-the-pete commented May 15, 2019 via email

@pete-the-pete
Copy link
Contributor Author

@hjdivad tried again, no test failures, but I do see this:
Screen Shot 2019-05-15 at 9 15 14 PM

@runspired
Copy link
Contributor

@pete-the-pete usually I find this means the packaging step produced bad packages. If you run that command with DEBUG=test-external as the env you will get a detailed log with info about what might have gone wrong.

@pete-the-pete
Copy link
Contributor Author

😢 still beyond my understanding...
Screen Shot 2019-05-16 at 2 23 04 PM

@pete-the-pete
Copy link
Contributor Author

thanks @runspired! i learned™, and now i do have test failures!

@runspired
Copy link
Contributor

I looked at what the test failures in M3 were and I suspect the issue is that we now access some of the "Event" properties to check for presence of methods in a way we did not before, which is trapped by M3's proxy handler. We may want to guard our checks to only apply to instances of DS.Model.

@pete-the-pete pete-the-pete force-pushed the RFC-0329 branch 2 times, most recently from 85461de to e4ed19a Compare May 20, 2019 04:30
@runspired runspired mentioned this pull request Jun 28, 2019
@pete-the-pete pete-the-pete reopened this Jul 2, 2019
@runspired runspired merged commit e574769 into emberjs:master Jul 3, 2019
@pete-the-pete pete-the-pete deleted the RFC-0329 branch July 3, 2019 22:40
pete-the-pete added a commit to pete-the-pete/data that referenced this pull request Jul 23, 2019
HeroicEric added a commit to HeroicEric/data that referenced this pull request Aug 30, 2019
Related RFC: https://github.com/emberjs/rfcs/blob/master/text/0329-deprecated-ember-evented-in-ember-data.md
Original PR: emberjs#6059

Deprecations were added for the following model lifecycle events
`becameError`, `becameInvalid`, `didCreate`, `didDelete`, `didLoad`,
`didUpdate`, `ready`, `rolledBack` in emberjs#6059.

There was a discussion on the PR that suggested adding an additional
check to make sure that that the deprecations were only triggered for
instances of Ember Data's `Model` class emberjs#6059 (comment)

This extra check was added but it accidentally was checking for `this`
to be an instance of the `Model` class, but that can never be true since
`this` will always be an instance of `InternalModel`:

https://github.com/emberjs/data/blob/5e495394cbf67025be2622f604e67004d79bc21c/packages/store/addon/-private/system/model/internal-model.ts#L446-L455

Since we only want to check for these deprecations for instances of
`Model`, I moved the deprecation logic to the `Model` class

emberjs#6059 also included a mechanism for
tracking deprecations that were logged per model class so we could
prevent logging multiple deprecations for the same model class and
deprecated lifecycle method pair:

https://github.com/emberjs/data/blob/5e495394cbf67025be2622f604e67004d79bc21c/packages/store/addon/-private/system/model/internal-model.ts#L61-L77

This also fixes a bug with the deprecation tracking logic which was that
we were never actually adding the logged deprecations to the `WeakMap`
meant to keep track of them.
HeroicEric added a commit to HeroicEric/data that referenced this pull request Aug 30, 2019
Related RFC: https://github.com/emberjs/rfcs/blob/master/text/0329-deprecated-ember-evented-in-ember-data.md
Original PR: emberjs#6059

Deprecations were added for the following model lifecycle events
`becameError`, `becameInvalid`, `didCreate`, `didDelete`, `didLoad`,
`didUpdate`, `ready`, `rolledBack` in emberjs#6059.

There was a discussion on the PR that suggested adding an additional
check to make sure that that the deprecations were only triggered for
instances of Ember Data's `Model` class emberjs#6059 (comment)

This extra check was added but it accidentally was checking for `this`
to be an instance of the `Model` class, but that can never be true since
`this` will always be an instance of `InternalModel`:

https://github.com/emberjs/data/blob/5e495394cbf67025be2622f604e67004d79bc21c/packages/store/addon/-private/system/model/internal-model.ts#L446-L455

Since we only want to check for these deprecations for instances of
`Model`, I moved the deprecation logic to the `Model` class

emberjs#6059 also included a mechanism for
tracking deprecations that were logged per model class so we could
prevent logging multiple deprecations for the same model class and
deprecated lifecycle method pair:

https://github.com/emberjs/data/blob/5e495394cbf67025be2622f604e67004d79bc21c/packages/store/addon/-private/system/model/internal-model.ts#L61-L77

This also fixes a bug with the deprecation tracking logic which was that
we were never actually adding the logged deprecations to the `WeakMap`
meant to keep track of them.
HeroicEric added a commit to HeroicEric/data that referenced this pull request Aug 30, 2019
Related RFC: https://github.com/emberjs/rfcs/blob/master/text/0329-deprecated-ember-evented-in-ember-data.md
Original PR: emberjs#6059

Deprecations were added for the following model lifecycle events
`becameError`, `becameInvalid`, `didCreate`, `didDelete`, `didLoad`,
`didUpdate`, `ready`, `rolledBack` in emberjs#6059.

There was a discussion on the PR that suggested adding an additional
check to make sure that that the deprecations were only triggered for
instances of Ember Data's `Model` class emberjs#6059 (comment)

This extra check was added but it accidentally was checking for `this`
to be an instance of the `Model` class, but that can never be true since
`this` will always be an instance of `InternalModel`:

https://github.com/emberjs/data/blob/5e495394cbf67025be2622f604e67004d79bc21c/packages/store/addon/-private/system/model/internal-model.ts#L446-L455

Since we only want to check for these deprecations for instances of
`Model`, I moved the deprecation logic to the `Model` class

emberjs#6059 also included a mechanism for
tracking deprecations that were logged per model class so we could
prevent logging multiple deprecations for the same model class and
deprecated lifecycle method pair:

https://github.com/emberjs/data/blob/5e495394cbf67025be2622f604e67004d79bc21c/packages/store/addon/-private/system/model/internal-model.ts#L61-L77

This also fixes a bug with the deprecation tracking logic which was that
we were never actually adding the logged deprecations to the `WeakMap`
meant to keep track of them.
HeroicEric added a commit to HeroicEric/data that referenced this pull request Aug 30, 2019
Related RFC: https://github.com/emberjs/rfcs/blob/master/text/0329-deprecated-ember-evented-in-ember-data.md
Original PR: emberjs#6059

Deprecations were added for the following model lifecycle events
`becameError`, `becameInvalid`, `didCreate`, `didDelete`, `didLoad`,
`didUpdate`, `ready`, `rolledBack` in emberjs#6059.

There was a discussion on the PR that suggested adding an additional
check to make sure that that the deprecations were only triggered for
instances of Ember Data's `Model` class emberjs#6059 (comment)

This extra check was added but it accidentally was checking for `this`
to be an instance of the `Model` class, but that can never be true since
`this` will always be an instance of `InternalModel`:

https://github.com/emberjs/data/blob/5e495394cbf67025be2622f604e67004d79bc21c/packages/store/addon/-private/system/model/internal-model.ts#L446-L455

Since we only want to check for these deprecations for instances of
`Model`, I moved the deprecation logic to the `Model` class

emberjs#6059 also included a mechanism for
tracking deprecations that were logged per model class so we could
prevent logging multiple deprecations for the same model class and
deprecated lifecycle method pair:

https://github.com/emberjs/data/blob/5e495394cbf67025be2622f604e67004d79bc21c/packages/store/addon/-private/system/model/internal-model.ts#L61-L77

This also fixes a bug with the deprecation tracking logic which was that
we were never actually adding the logged deprecations to the `WeakMap`
meant to keep track of them.
HeroicEric added a commit to HeroicEric/data that referenced this pull request Sep 2, 2019
Related RFC: https://github.com/emberjs/rfcs/blob/master/text/0329-deprecated-ember-evented-in-ember-data.md
Original PR: emberjs#6059

Deprecations were added for the following model lifecycle events
`becameError`, `becameInvalid`, `didCreate`, `didDelete`, `didLoad`,
`didUpdate`, `ready`, `rolledBack` in emberjs#6059.

There was a discussion on the PR that suggested adding an additional
check to make sure that that the deprecations were only triggered for
instances of Ember Data's `Model` class emberjs#6059 (comment)

This extra check was added but it accidentally was checking for `this`
to be an instance of the `Model` class, but that can never be true since
`this` will always be an instance of `InternalModel`:

https://github.com/emberjs/data/blob/5e495394cbf67025be2622f604e67004d79bc21c/packages/store/addon/-private/system/model/internal-model.ts#L446-L455

Since we only want to check for these deprecations for instances of
`Model`, I moved the deprecation logic to the `Model` class

emberjs#6059 also included a mechanism for
tracking deprecations that were logged per model class so we could
prevent logging multiple deprecations for the same model class and
deprecated lifecycle method pair:

https://github.com/emberjs/data/blob/5e495394cbf67025be2622f604e67004d79bc21c/packages/store/addon/-private/system/model/internal-model.ts#L61-L77

This also fixes a bug with the deprecation tracking logic which was that
we were never actually adding the logged deprecations to the `WeakMap`
meant to keep track of them.
igorT pushed a commit that referenced this pull request Sep 5, 2019
Related RFC: https://github.com/emberjs/rfcs/blob/master/text/0329-deprecated-ember-evented-in-ember-data.md
Original PR: #6059

Deprecations were added for the following model lifecycle events
`becameError`, `becameInvalid`, `didCreate`, `didDelete`, `didLoad`,
`didUpdate`, `ready`, `rolledBack` in #6059.

There was a discussion on the PR that suggested adding an additional
check to make sure that that the deprecations were only triggered for
instances of Ember Data's `Model` class #6059 (comment)

This extra check was added but it accidentally was checking for `this`
to be an instance of the `Model` class, but that can never be true since
`this` will always be an instance of `InternalModel`:

https://github.com/emberjs/data/blob/5e495394cbf67025be2622f604e67004d79bc21c/packages/store/addon/-private/system/model/internal-model.ts#L446-L455

Since we only want to check for these deprecations for instances of
`Model`, I moved the deprecation logic to the `Model` class

#6059 also included a mechanism for
tracking deprecations that were logged per model class so we could
prevent logging multiple deprecations for the same model class and
deprecated lifecycle method pair:

https://github.com/emberjs/data/blob/5e495394cbf67025be2622f604e67004d79bc21c/packages/store/addon/-private/system/model/internal-model.ts#L61-L77

This also fixes a bug with the deprecation tracking logic which was that
we were never actually adding the logged deprecations to the `WeakMap`
meant to keep track of them.
igorT pushed a commit that referenced this pull request Sep 6, 2019
Related RFC: https://github.com/emberjs/rfcs/blob/master/text/0329-deprecated-ember-evented-in-ember-data.md
Original PR: #6059

Deprecations were added for the following model lifecycle events
`becameError`, `becameInvalid`, `didCreate`, `didDelete`, `didLoad`,
`didUpdate`, `ready`, `rolledBack` in #6059.

There was a discussion on the PR that suggested adding an additional
check to make sure that that the deprecations were only triggered for
instances of Ember Data's `Model` class #6059 (comment)

This extra check was added but it accidentally was checking for `this`
to be an instance of the `Model` class, but that can never be true since
`this` will always be an instance of `InternalModel`:

https://github.com/emberjs/data/blob/5e495394cbf67025be2622f604e67004d79bc21c/packages/store/addon/-private/system/model/internal-model.ts#L446-L455

Since we only want to check for these deprecations for instances of
`Model`, I moved the deprecation logic to the `Model` class

#6059 also included a mechanism for
tracking deprecations that were logged per model class so we could
prevent logging multiple deprecations for the same model class and
deprecated lifecycle method pair:

https://github.com/emberjs/data/blob/5e495394cbf67025be2622f604e67004d79bc21c/packages/store/addon/-private/system/model/internal-model.ts#L61-L77

This also fixes a bug with the deprecation tracking logic which was that
we were never actually adding the logged deprecations to the `WeakMap`
meant to keep track of them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-review Tracks PRs that require a code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants