janmisek / ember-error-handler

Error handling for ambitious web applications
MIT License
4 stars 2 forks source link

Remove dependency on ember-exex and ember-types #2

Open rogatty opened 7 years ago

rogatty commented 7 years ago

Hi, it's a great addon, very helpful! The thing which bothers me is the dependency on ember-exex and ember-types (there is a mistake in package.json, these two should be in the dependencies section), which are not-so-small. It seems to me that their use is not critical, just convenient. Would you consider removing the dependency or making it optional / configurable?

janmisek commented 7 years ago

Hi I am glad addon is helpfull.

I have fixed dependency tree.

Mentioned dependencies are not essentially tight with addon responsibility but I would not categorize them as convenient only. ember-exex provides custom error classes which are important by my opinion to any sophisticated software and ember-types is used to extract data about thrown entity - sometimes string or undefined is thrown. Therefore to remove dependencies I would have to duplicate code from addons into ember-error-handler iteself.

Also what do you mean by not-so-small ? these are standard ember addons with nothing more then ember-cli tooling.

rogatty commented 7 years ago

After fully implementing ember-error-handler in our app I agree that ember-exex is a nice addition and contains multiple tools which otherwise we would have to handle ourselves. These two are a nice combo for handling errors.

That being said, ember-types adds 27 files to vendor.js which, after transpiling to ES5 and before gzipping are:

All we use in ember-error-handler is one, simple method import {extractName} from 'ember-types/classes'. Please consider extracting it somehow to not have this dependency.

Until then, I'm going to use this method of filtering the vendor tree in our ember-cli-build.js:

EmberApp.prototype.addonTreesFor = function (type) {
  return this.project.addons.map(function (addon) {
    if (addon.treeFor) {
      var tree = addon.treeFor(type);

      if (tree) {
        // uncomment to see the files available to be filtered out
        // tree = stew.log(tree, {output: 'tree'});
        tree = stew.rm(tree,
          'modules/ember-types/asserts/**/*.js',
          'modules/ember-types/constants/*.js',
          'modules/ember-types/property/*.js'
        );
      }

      return tree;
    }
  }).filter(Boolean);
};

Thanks again, good stuff!

janmisek commented 6 years ago

ember-types has been in latest release removed, ember-exex will follow.

buzzware commented 6 years ago

I agree that Error subclasses are important, but its a matter of which base class, and I don't think that should be dictated by an error handler library, since it must handle all errors anyway. If this package must have an extendable error base class, this seems like a good one, covering node, es6, es5 and typescript https://www.npmjs.com/package/ts-error however I had an error trying to import it (perhaps my bad). Please remove ember-exex.