Does not allow or do anything about duplicate definitions

[ajcrites]

Because of the way the cache is used, if you try to define an error with the same name twice error-factory will return the first definition quietly.

https://gist.github.com/ajcrites/47d06390bab8bbe6e7cb

If you try to redefine an error with the same name, I think it should either work or an exception should be thrown for the duplicate name.

Simply checking for other arguments would imply a duplicate definition to me but you could also use deep-equals or something to check that the existing definition is different -- I think this complexity is unnecessary, though.

var One = errorFactory("one", ["first dfn"]);
// Existence of second argument implies redefinition which
// should probably just be an error
var Two = errorFactory("one", ["second dfn"]);

[yanickrochon]

Yes, this is a problem. I tried eliminating that situation by adding namespaces to custom errors. This is actually documented.

Because errors are cached, projects should use namespaced errors to avoid mistakenly returning an error already defined somewhere else, with possibly different parameters, etc. As a rule of thumb, non-namespaced errors should not define named arguments, or use message templates, and should be reserved as low-level error types only.

A solution could be to add a flag that would throw an error if trying to redeclare an already declared custom error. For example :

var CustomError1 = errorFactory('CustomError', options);
var CustomError2 = errorFactory('CustomError', otherOptions);   // silent
var CustomError2 = errorFactory('CustomError');   // ok, return cached type

errorFactory.throwOnRedifinition = true;   // draft property name, open to suggestions

var CustomError3 = errorFactory('CustomError');   // ok, return cached type
var CustomError4 = errorFactory('CustomError', otherOptions);   // throw error

What do you think?

[yanickrochon]

Another option could be to add a function to check for the existence of the custom error type.

if (!errorFactory.exists('CustomError')) {
  errorFactory('CustomError', options);  // define error
}

Either way, simply throwing something on redefinition by default is not going to happen. Either for backward compatibility, and because validating that both definitions are actually different is simply impossible, and because this is documented already.

[yanickrochon]

A third option, which I do not personally like, but I'm stating it anyway, would be return the custom type, but without caching or overwriting the existing cached one. For example :

var CustomError1 = errorFactory('CustomError', options);
var CustomError2 = errorFactory('CustomError', otherOptions);

var CustomError3 = errorFactory('CustomError');

CustomError1 === CustomError2;  // false
CustomError1 === CustomError3;  // true
CustomError2 === CustomError3;  // false

[ajcrites]

These are all good options. I think that a fourth possibility is that adding new options should overwrite the cache, so same as your latest example except

CustomError2 === CustomError3; // true

I'm actually not sure what the best course of action should be and it would really depend on the overall use case for duplicate custom error definitions (usually a per-project thing as you've indicated -- namespacing does indeed help mitigate this).

Another possible option is that you can throw an error or something on retrieval of multiple custom definitions (in the case of CustomError3 above). I know you don't like the backwards compatibility breakage, but my answer to that is that semantic versioning exists for a reason.

Honestly these are all valid options to me, so I would want more input from other people before making a decision absolutely. I think what makes the most sense to me is to enable some flag for debugging that would check for duplicate definitions and either emit a message or throw an error if this happens. This gives the most flexibility.

[yanickrochon]

In case of adding more options to a custom error, I strongly suggest you extend your types instead. This was discussed in issue #2.

There is also the option of disabling the cache altogether. Thus eliminating any possible wrong definitions. However, doing so would prevent from retrieving a previously declared error type, and could cause problems for existing projects.

Personally, I don't see any problem with the current implementation. I am an advocate of "don't code like an arse, and avoid messing other people's code". Using namespaced error types, it is very unlikely that you'd get the wrong error type back (unless you or someone else messed up).. However, I am open to suggestions and for a clear and solid case on a solution, which does not involve inserting too much complexity and other issues.