can.map throws strings, not errors. #1773

Closed
akagomez opened this Issue Jul 8, 2015 · 9 comments

Comments

Projects
None yet
4 participants
@akagomez
Contributor

akagomez commented Jul 8, 2015

Currently:

throw "can.Map: Object does not exist";

Should be:

throw Error("can.Map: Object does not exist");

https://github.com/bitovi/canjs/blob/master/map/map.js#L495

/cc @imjoshdean

@imjoshdean

This comment has been minimized.

Show comment
Hide comment
@imjoshdean

imjoshdean Jul 8, 2015

Contributor

👍 Throwing strings don't provide things like stack traces and other Error goodies that are useful.

Contributor

imjoshdean commented Jul 8, 2015

👍 Throwing strings don't provide things like stack traces and other Error goodies that are useful.

@imjoshdean imjoshdean self-assigned this Jul 8, 2015

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jul 8, 2015

Contributor

👍 But I'm wondering if this could be considered a breaking API change?

Contributor

daffl commented Jul 8, 2015

👍 But I'm wondering if this could be considered a breaking API change?

@akagomez

This comment has been minimized.

Show comment
Hide comment
@akagomez

akagomez Jul 8, 2015

Contributor

Other instances I found:

./map/define/define_test.js:546:                        throw '"foo"\'s value method should not be called.';
./map/lazy/lazy.js:196:             throw "can.LazyMap: object does not exist";
./map/map.js:495:                       throw "can.Map: Object does not exist";
./util/fixture/fixture.js:108:                  throw "fixtures.js Error " + error + " " + message;
./view/parser/parser.js:236:                throw "Parse Error: " + html;
./view/view.js:46:          throw "can.view: No template or empty template:" + url;
Contributor

akagomez commented Jul 8, 2015

Other instances I found:

./map/define/define_test.js:546:                        throw '"foo"\'s value method should not be called.';
./map/lazy/lazy.js:196:             throw "can.LazyMap: object does not exist";
./map/map.js:495:                       throw "can.Map: Object does not exist";
./util/fixture/fixture.js:108:                  throw "fixtures.js Error " + error + " " + message;
./view/parser/parser.js:236:                throw "Parse Error: " + html;
./view/view.js:46:          throw "can.view: No template or empty template:" + url;
@imjoshdean

This comment has been minimized.

Show comment
Hide comment
@imjoshdean

imjoshdean Jul 8, 2015

Contributor

I...don't think so? How would it break the API?

Contributor

imjoshdean commented Jul 8, 2015

I...don't think so? How would it break the API?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jul 8, 2015

Contributor

It would not. We don't document these errors.

Contributor

justinbmeyer commented Jul 8, 2015

It would not. We don't document these errors.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jul 10, 2015

Contributor

Shouldn't it be throw new Error("can.Map: Object does not exist");? Anyway, if it doesn't we can even get it into the 2.2.7 patch release.

Contributor

daffl commented Jul 10, 2015

Shouldn't it be throw new Error("can.Map: Object does not exist");? Anyway, if it doesn't we can even get it into the 2.2.7 patch release.

@daffl daffl added this to the 2.2.8 milestone Jul 24, 2015

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jul 28, 2015

Contributor

@akagomez can you submit a pull request for this? Thanks!

Contributor

justinbmeyer commented Jul 28, 2015

@akagomez can you submit a pull request for this? Thanks!

@justinbmeyer justinbmeyer added the bug label Jul 28, 2015

@imjoshdean

This comment has been minimized.

Show comment
Hide comment
@imjoshdean

imjoshdean Jul 28, 2015

Contributor

@justinbmeyer I'm submitting one shortly, with more than just this throwing an error instead of a new string.

Contributor

imjoshdean commented Jul 28, 2015

@justinbmeyer I'm submitting one shortly, with more than just this throwing an error instead of a new string.

@akagomez

This comment has been minimized.

Show comment
Hide comment
@akagomez

akagomez Jul 30, 2015

Contributor

The PR: #1811

Contributor

akagomez commented Jul 30, 2015

The PR: #1811

imjoshdean added a commit that referenced this issue Aug 28, 2015

imjoshdean added a commit that referenced this issue Aug 28, 2015

Merge pull request #1869 from bitovi/shoretel-throw-errors
Fixes #1773, throw Error objects rather than strings.

@imjoshdean imjoshdean closed this in 2a2c38e Sep 8, 2015

daffl added a commit that referenced this issue Sep 8, 2015

Merge pull request #1811 from bitovi/1773_throwErrorObject
Fixes #1773, throw Error objects rather than strings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment