rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

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

Open runspired opened this issue 6 years ago • 2 comments

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;

runspired avatar Nov 20 '18 18:11 runspired

@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

pixelhandler avatar Apr 26 '19 20:04 pixelhandler

@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 avatar Apr 26 '19 23:04 runspired

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

runspired avatar Sep 01 '23 17:09 runspired