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] Add ducktyping of AdapterError #4111

Merged
merged 1 commit into from
Jan 27, 2016
Merged

[BUGFIX beta] Add ducktyping of AdapterError #4111

merged 1 commit into from
Jan 27, 2016

Conversation

danmcclain
Copy link
Contributor

There are instanceof checks for AdapterError; this removes it in favor of checking for a property on the error. isAdapterError appears on all errors that use AdapterError as the base.

This PR is looking to avoid the need to import private errors in fastboot to keep the same code flow in the ajax method used in node land, related PR: ember-fastboot/ember-cli-fastboot#95

@danmcclain
Copy link
Contributor Author

Tests were failing locally even when reverting so I figured I'd try pushing anyway. Will dig back in tomorrow morning. Would like feedback on direction as well

/cc @rwjblue

@rwjblue
Copy link
Member

rwjblue commented Jan 22, 2016

Using flags seems better than using instance of to me...

@danmcclain danmcclain changed the title Add ducktyping of AdapterError and InvalidError Add ducktyping of AdapterError Jan 22, 2016
@danmcclain
Copy link
Contributor Author

Slightly confusing, there are 2 different definitions for InvalidError, changed this so it only handles AdapterError which is the one that fastboot is checking for ultimately

@bmac
Copy link
Member

bmac commented Jan 22, 2016

there are 2 different definitions for InvalidError

@tchak is this an oversite or do we need 2 definitions for InvalidError?

@bmac
Copy link
Member

bmac commented Jan 22, 2016

@danmcclain Is this something that should be considered a [BUGFIX beta] so it can be included in the 2.4 release?

@danmcclain
Copy link
Contributor Author

@bmac Here are the 2 different definitions:
Adapter InvalidError:

export function InvalidError(errors) {
assert('`InvalidError` expects json-api formatted errors array.', Ember.isArray(errors || []));
AdapterError.call(this, errors, 'The adapter rejected the commit because it was invalid');
}
InvalidError.prototype = Object.create(AdapterError.prototype);

Model InvalidError:
https://github.com/emberjs/data/blob/209326f22324d8e29eee0207908d02725f7cb83e/addon/-private/system/model/errors/invalid.js

Also, I can label as such, I didn't give it a label, as I was unsure what you would want it to be

@danmcclain
Copy link
Contributor Author

@bmac I think [BUGFIX beta] would apply. Labeling as such now

@danmcclain danmcclain changed the title Add ducktyping of AdapterError [BUGFIX beta] Add ducktyping of AdapterError Jan 23, 2016
@tchak
Copy link
Member

tchak commented Jan 25, 2016

I am not a big fan of this. I started some work to make it easy to inherit from AdapterError which seems to me like a better practice. But I do understand that it makes life easier in some cases, so I guess it's 👍

The two definitions of InvalidError is definitely a bug/oversight.

@rwjblue
Copy link
Member

rwjblue commented Jan 26, 2016

@tchak Using instanceof has generally been problematic for us in Ember, so we have been switching to using flags like this.

@@ -853,7 +853,7 @@ export default Adapter.extend(BuildURLMixin, {
requestData
);

if (response instanceof AdapterError) {
if (response.isAdapterError) {
Copy link
Member

Choose a reason for hiding this comment

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

Should likely be response && response.isAdapterError in case response is undefined / null.

@bmac
Copy link
Member

bmac commented Jan 27, 2016

@danmcclain @rwjblue added one final comment. Once that is fixed this pr should be ready to be merged.

@danmcclain
Copy link
Contributor Author

@bmac will update shortly 👍🏼

There are `instanceof` checks for `AdapterError` this removes them in
favor of checking for a property on the error, `isAdapterError` appears
on all errors that use `AdapterError` as the base.
@danmcclain
Copy link
Contributor Author

@bmac updated per @rwjblue's comment

bmac added a commit that referenced this pull request Jan 27, 2016
[BUGFIX beta] Add ducktyping of `AdapterError`
@bmac bmac merged commit 6284d57 into emberjs:master Jan 27, 2016
@bmac
Copy link
Member

bmac commented Jan 27, 2016

Thanks @danmcclain

@tchak
Copy link
Member

tchak commented Jan 28, 2016

@rwjblue ok. I see quite well why instanceof can be problematic. It just makes me very sad... But such is life :)

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

4 participants