tildeio/router.js

TransitionAborted should be a subclass of Error, not a POJO

Closed this issue · 12 comments

lolmaus commented

https://github.com/tildeio/router.js/blob/master/lib/router/transition.js#L314-L317

Ember throws TransitionAborted objects as errors, but their API is inconsistent with Error (e. g. no stack property and .toString() is ridiculously non-informative).

To resolve this, TransitionAborted should be derived from the native Error object or one of its subclasses.

rwjblue commented

FWIW - We tweaked the Ember implementation in 2.10+, and we should use that instead:

https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/error.js#L11

lolmaus commented

@rwjblue Uhm, should router.js depend on Ember? From Bower or NPM?

If router.js is meant to be a standalone library, it shouldn't be that biased... I think... But I don't know how to import modules conditionally with ES2015. Could probably revert to require?

Please advise.

rwjblue commented

Hehe, I wasn't saying to use Ember.Error. I was suggesting to copy that implementation...

lolmaus commented

@rwjblue I suggest that we take example from Chai's assertion-error:

  1. Give EmberError a new, unbranded name.
  2. Extract it into an independent package.
  3. Reuse the package in Ember and router.js.
nathanhammond commented

@lolmaus I'm not opposed to this but it takes much more coordination and effort. If you're volunteering then let's explore it...

See also @sandstrom's PR.

lolmaus commented

@sandstrom's PR does solve this issue and it copies Ember's implementation as @rwjblue suggests.

I have one tiny objection though: the PR violates DRY by offering two error types that share the same implementation:
https://github.com/sandstrom/router.js/blob/45dfe284bd293451a7c3d703284a63ff958db706/lib/router/transition-aborted-error.js
https://github.com/sandstrom/router.js/blob/45dfe284bd293451a7c3d703284a63ff958db706/lib/router/unrecognized-url-error.js

The only difference is the name of the error. So it does make sense to extract this implementation into a reusable module.

I'd gladly do it, but I'm an Ember CLI child and it beats me how a package should look like to be consumed by such projects as ember.js and router.js. Please explain the following:

  • Can I use ES2015 class? This F.prototype= thing is super old.
  • If yes, is it possible to use pure ES2015, assuming either Node 6 or Babel in the consuming app? Or should the package bundle transpiled artifacts?
  • If transpiled, may I use a boilerplate such as babel-starter-kit and generator-babel-boilerplate?
  • What module format(s) should the package provide? Can it be just a naked ES2015 module file? Or should the package include an UMD-wrapped artifact?
sandstrom commented

@lolmaus I don't think the repetition is a problem.

This post more or less explain my thinking:
http://thereignn.ghost.io/on-dry-and-the-cost-of-wrongful-abstractions/

lolmaus commented

to dry or not to dry@sandstrom It's a rather long article, but it only provides one argument against DRY: the cost of abstraction can be higher than the cost of violating DRY.

The cost of abstraction is composed of a single element:

  • making code hard to understand (when it has many layers of abstraction).

The cost of violating DRY is composed of many elements:

  • increasing tech debt,
  • sacrificing maintainability,
  • making it easier for dead code to remain,
  • introducing bugs (due to fixing not all the copies),
  • making it harder to reuse the code,
  • etc.

Here's what the article suggests as a rule of thumb:

...if the abstraction you wrote requires someone new on the project to spend hours understanding it, then you're probably doing something wrong.

I don't think that the following will take hours to understand:

// transition-aborted-error.js

import CrossPlatformError from 'cross-platform-error';

export class TransitionAbortedError extends CrossPlatformError {
  defaultMessage = 'TransitionAborted';
}

On the opposite, it improves understanding in two important ways:

  1. Thanks to a meaningful parent class name, the purpose of the custom error class becomes clear.
  2. It becomes visible how exactly child error classes are different from each other and whether they bear any unique behavior.

Predict whether code duplication will happen again on that particular portion or not and decide accordingly.

We're already using this cross-platform error implementation in at least two codebases. The implementation exists for a reason and it's perfectly rational to expose it as a module so that any other isomorphic project can reuse it rather than reinvent the wheel.

sandstrom commented

@lolmaus Thanks for introduction to DRY, but I like to think that I have a decent understanding of the principle itself. We just disagree on how/when to apply it.

But let's not discuss this further. Our time is better spent on other things. ⛵️

lolmaus commented

It wasn't a lecture. I provided argumentation. ¯_(ツ)_/¯

sandstrom commented

This was solved in #204