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.

Sign up for GitHub

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

Jump to bottom

Update Ember.deprecate and Ember.warn calls to include required information #3570

Merged
merged 1 commit into from
Jul 27, 2015
Merged

Update Ember.deprecate and Ember.warn calls to include required information #3570

merged 1 commit into from
Jul 27, 2015

Conversation

HeroicEric
Copy link
Member

  • Update calls to Ember.deprecate to include required options
  • Update calls to Ember.warn to include required options

fixes #3568

@HeroicEric HeroicEric changed the title [WIP] Update ember deprecate and warn calls [WIP] Update Ember.deprecate and Ember.warn calls to include required information Jul 22, 2015
@HeroicEric HeroicEric force-pushed the update-ember-deprecate-and-warn-calls branch 2 times, most recently from 9fc6689 to 507ae2a Compare July 22, 2015 03:06
@HeroicEric HeroicEric changed the title [WIP] Update Ember.deprecate and Ember.warn calls to include required information Update Ember.deprecate and Ember.warn calls to include required information Jul 22, 2015
@HeroicEric
Copy link
Member Author

I wasn't really sure how to come up with the ids so let me know if there is a better pattern to follow. Same with the until attrs.

@HeroicEric HeroicEric force-pushed the update-ember-deprecate-and-warn-calls branch 2 times, most recently from e7d362b to d46386e Compare July 22, 2015 03:14
@wecc
Copy link
Contributor

wecc commented Jul 22, 2015

@HeroicEric I'd say we should keep the IDs simple, like group.deprecation. So instead of store.adapter-for.passing-class-to-store-method maybe just store.passing-class-to-store-methods? I also think the system part of system.model.inverse-for-without-inverse-option is too explicit, (could be just model.reflexive-relationship-without-inverse)

@rwjblue would it make sense to prefix all ED deprecation/warn IDs with ds.?

@rwjblue
Copy link
Member

rwjblue commented Jul 22, 2015

Yeah, I think prefixing the id with an identifier for the project is a good idea. It will help the tooling that handles this know where (ember, ember-data, etc) a given deprecation comes from.

@rwjblue
Copy link
Member

rwjblue commented Jul 22, 2015

I'd love to get this into the next point release, if possible. I'm working on tooling to help folks deal with these things and having the id's and until values make things much easier...

wecc
wecc reviewed Jul 22, 2015
View reviewed changes
packages/ember-data/lib/adapters/errors.js
@@ -79,7 +79,7 @@ AdapterError.prototype = Object.create(EmberError.prototype);
*/
export function InvalidError(errors) {
if (!Ember.isArray(errors)) {
Ember.deprecate('`InvalidError` expects json-api formatted errors.');
Ember.deprecate('`InvalidError` expects json-api formatted errors.', false, { id: 'errors.not-json-api-format', until: '3.0.0' });
Copy link
Contributor

Choose a reason for hiding this comment

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

ds.errors.invalid-error-expects-json-api-format?

@wecc
Copy link
Contributor

wecc commented Jul 22, 2015

I've added some suggestions that I think make sense. Feel free to use them or not. They all follow the ds.module.deprecation pattern.

Would also love to see this get into 1.13.7 :shipit:

@HeroicEric HeroicEric force-pushed the update-ember-deprecate-and-warn-calls branch 2 times, most recently from c21650a to 71a2a9a Compare July 22, 2015 20:28
HeroicEric
HeroicEric reviewed Jul 22, 2015
View reviewed changes
packages/ember-data/lib/system/store.js
modelName = modelOrClass;
} else {
Ember.deprecate(`Passing classes to store methods has been removed. Please pass a dasherized string instead of ${Ember.inspect(modelName)}`, false, { id: 'ds.store.passing-classes-deprecated', until: '3.0.0' });
Copy link
Member Author

Choose a reason for hiding this comment

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

do these ids need to be totally unique? Like, can this id be the same as the one in serializerFor?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say they can be the same since they are about the exact same thing, just added in multiple places. @rwjblue?

Copy link
Member

Choose a reason for hiding this comment

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

Absolutely, it isn't about being unique at all. Its about being able to deal with specific things. If two Ember.deprecate invocations are for the same underlying thing, we should use the same id.

@HeroicEric
Copy link
Member Author

@wecc thanks. Those are much more consistent 👍

@HeroicEric HeroicEric force-pushed the update-ember-deprecate-and-warn-calls branch from 71a2a9a to 4a39415 Compare July 22, 2015 20:34
wecc
wecc reviewed Jul 22, 2015
View reviewed changes
packages/ember-data/lib/system/relationships/ext.js
@@ -256,6 +254,13 @@ Model.reopenClass({
inverseKind = inverse.kind;
} else {
//No inverse was specified manually, we need to use a heuristic to guess one
if (propertyMeta.type === propertyMeta.parentType.modelName) {
Ember.warn(`Detected a reflexive relationship by the name of '${name}' without an inverse option. Look at http://emberjs.com/guides/models/defining-models/#toc_reflexive-relation for how to explicitly specify inverses.`, false, {
id: 'ds.model.relflexive-relationship-without-inverse',
Copy link
Contributor

Choose a reason for hiding this comment

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

typo, relflexive

@HeroicEric HeroicEric force-pushed the update-ember-deprecate-and-warn-calls branch from 4a39415 to b4a2047 Compare July 22, 2015 20:45
@wecc
Copy link
Contributor

wecc commented Jul 22, 2015

I was thinking about until: '3.0.0'. I think some of these deprecations should be removed for 2.0 and are leftovers from 1.13 not cleaned up. Is the ember-debug-handlers feature present in Ember 1.13.x? If so I think that there are a lot more deprecations in the release branch that we might need a separate PR for with until: '2.0.0'.

@rwjblue
Copy link
Member

rwjblue commented Jul 22, 2015

@wecc - See emberjs/ember-inspector#430 for details, but I made a polyfill for this to run against older Ember versions.

@HeroicEric
Copy link
Member Author

@wecc can you mark anything in here that should be marked 2.0.0 and I can work on a PR for 1.13.x

@HeroicEric HeroicEric mentioned this pull request Jul 22, 2015
Merged
@HeroicEric
Copy link
Member Author

Will the the changes from #3575 (bugfix release) make their way into master or should I continue with this PR?

@bmac
Copy link
Member

bmac commented Jul 24, 2015

@HeroicEric you should keep working on this pr. usually things go from master to release but not the other way around.

@HeroicEric HeroicEric force-pushed the update-ember-deprecate-and-warn-calls branch 3 times, most recently from eb11ab3 to 41176b6 Compare July 26, 2015 15:14
@HeroicEric
Copy link
Member Author

I think this is good to go now.

@wecc
Copy link
Contributor

wecc commented Jul 26, 2015

LGTM! Can you prefix with [BUGFIX beta]?

@HeroicEric HeroicEric force-pushed the update-ember-deprecate-and-warn-calls branch from 41176b6 to 5ba8df8 Compare July 26, 2015 17:13
bmac added a commit that referenced this pull request Jul 27, 2015
@bmac
Merge pull request #3570 from HeroicEric/update-ember-deprecate-and-w…
d842890 
…arn-calls

Update `Ember.deprecate` and `Ember.warn` calls to include required information
@bmac bmac merged commit d842890 into emberjs:master Jul 27, 2015
@bmac
Copy link
Member

bmac commented Jul 27, 2015

Thanks.

@HeroicEric HeroicEric deleted the update-ember-deprecate-and-warn-calls branch July 27, 2015 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update Ember.deprecate and Ember.warn calls to include required options.
4 participants
@HeroicEric @wecc @rwjblue @bmac