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

[DATA] Deprecate extending Native Error (AdapterError and subclasses) #402

Closed
runspired opened this issue Nov 20, 2018 · 3 comments
Closed
Labels
T-deprecation T-ember-data RFCs that impact the ember-data library

Comments

@runspired
Copy link
Contributor

Alternative API would be something like isAdapterError(thing) and buildAdapterError(options), the only real requirement is the errors array on the Error, and possibly a property to specify it as an adapterError. All current subclasses could implement some form of code or type in addition to their custom message, but we would only ever use native errors. ala

let error = new Error(message);

Object.assign(error, {
  isAdapterError: true,
  ...whateverElseGoesHere
});
throw error;
@pixelhandler
Copy link

pixelhandler commented Apr 26, 2019

@runspired this is the list of errors I see in the docs (3.9) and a few example uses ...

DS.AdapterError docs

Subclasses:

  • DS.InvalidError docs
  • DS.TimeoutError docs
  • DS.AbortError docs
  • DS.UnauthorizedError 401
  • DS.ForbiddenError 403
  • DS.NotFoundError 404
  • DS.ConflictError 409
  • DS.ServerError 500

Various uses...

Custom error extending

import DS from 'ember-data';

export default DS.AdapterError.extend({ message: "Down for maintenance." });
new MaintenanceError();

Condition to match error type

error instanceof TimeoutError

So we'd want to keep the above behavior, and no longer extend EmberError as there is no need, it references Error type in JS.

Also we'd want to use typical JS custom errors, Error#Custom_Error_Types

Properties we'd add to the Adapter Errors:

isAdapterError: true;
errors: Array<{title:string,detail:string}>
number: number;
code: number;

Properties based on Error prototype...

message: string; // set by constructor
stack: string;
fileName: string;
lineNumber: number;
name: string;

Existing source for AdapterError


Perhaps using class syntax is the way to start...

class AdapterError extends Error {
  constructor(errors = [{title: 'AdapterError', detail: 'Adapter operation failed'}]) {
    super();
    if (Error.captureStackTrace) {
      Error.captureStackTrace(this, AdapterError);
    }
    this.message = 'Adapter operation failed', 
    this.name = 'AdapterError';
    this.errors = errors;
  }
  static extend(props) {
    // logic for compatibility and deprecation warning
  }
}

class NotFoundError extends AdapterError {
  constructor(errors = [{title: 'Not Found', detail: 'Resource could not be found', status: '404'}]) {
    super(errors);
    this.message = 'Not Found';
    this.name = 'NotFoundError';
    this.code = 404;
  }
  static extend(props) {
    // logic for compatibility and deprecation warning
  }
}

I think it's important to keep support for type comparisons error instanceof NotFoundError vs error instanceof Error && error.isAdapterError && error.code === 404

@runspired
Copy link
Contributor Author

runspired commented Apr 26, 2019

@pixelhandler there's a couple of driving motivations for this but I'll list the biggest:

  1. eliminate extension of EmberError
  2. handle the in-extensibility of native Error.
  3. eliminate .extends(

Today we have to write very convoluted extension patterns that decrease the legibility of the errors printed to the console because the following does not work cross-platform (and may never)

class AdapterError extends Error {
  constructor() {
    super(...arguments);
    this.isAdapterError = true;
  }
}

Moving to a functional pattern is beneficial for performance and maintainability. An example API might be:

function createAdapterError(errors, message, statusCode, statusText) {
  const error = new Error(message);
  Object.assign(error, {
    isAdapterError: true,
    statusCode,
    statusText,
    errors
  });
  return error;
}

function is500Error(error) {
  return error.isAdapterError && error.statusCode === 500;
}

While at first the functional approach may seem to diminish value because we lose the instanceof check

  • a flag check is more performance than instanceof
  • it makes switch and if statements work more nicely, especially in templates
  • it plays nice with structural typing
  • flattens the class inheritance (less code to compile, parse, and ship!)

@runspired
Copy link
Contributor Author

This quirk is better deprecated by the broader deprecation of the adapter/serializer pattern that will occur once RequestManager is moved to the recommended stage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-deprecation T-ember-data RFCs that impact the ember-data library
Projects
Archived in project
Development

No branches or pull requests

3 participants