Better errors from visitable

[simonihmig]

This adds the actual error message from Ember's router to the (too generic) message from visitable. This is especially important when your test needs to know what kind of error was triggered. For example some tests might want to handle TransitionAborted errors differently, see emberjs/ember-test-helpers#332 (comment).

[ro0gr]

Hey @simonihmig 👋 Thanks!

I think I'm not a fan to add external error messages into the page object error messages at the first place. This may be a breaking change for some test suites if we merge it as is. And this can break test suites later, when someone upgrades the @ember/test-helpers. That was one of the reasons we've migrated to a page object's own error message before(#606), as a part of the v2 breaking realease.

I think a good enough tradeoff could be to keep the original error in the Error.cause. I believe, in this case the change can be backward compatible, and if someone really needs it they can use it(on their own risk), and it potentially allows for a better control over the original error type etc.. WDYT?

[ro0gr]

This may be a breaking change for some test suites if we merge it as is.

Ok I think I overrated the breaking nature of this change a bit. But I still feel like the cause is a proper place to store this kind of info.

[simonihmig]

Ok I think I overrated the breaking nature of this change a bit.

No worries! Normally I wouldn't think of the error message as something covered by the public API contract to be always the same. But actually, the changes in ecpo v2 were of a breaking nature to us because of this, as we had some checks like e.message === 'TransitionAborted' in place (for that very specific use case only) that broke with the upgrade.

I agree that we should make use of .cause, I forgot that this existed, so thanks for the suggestion, it totally makes sense!

However, I am still wondering if we should also update the message itself? You can only make use of .cause when actually running the test and putting a breakpoint somewhere to inspect it, right? But giving a better message in e.g. CI logs would be valuable for DX, to be able to immediately understand why the tests are failing. Could be an error thrown in a route hook, could be an error response in the (mocked) API, could be that transition error thing or whatnot...

But I'll leave that up to you to decide. My primary use case of being able to detect TransitionAborted cases is perfectly served with the .cause property! Will update this PR once this discussion has been settled! :)

[ro0gr]

But giving a better message in e.g. CI logs would be valuable for DX

I think you are right. Let's do it!

As a side note, I think it does also make sense to express the cause via jsdoc, so a future user is aware of this capability.

[simonihmig]

I just pushed this commit, which tries to implement the suggestion. However, there is a bit still left for discussion, because I realized that the error rethrown from the visitable helper is actually not the one received by the user, as it is catched and rethrown again by the "better-errors" functionality. And that already has opinions about cause here, which I didn't want to break by passing the original error. What I did instead was to just pass the message of e.cause if it exists, but keep the rest in place. Does that make sense?

[ro0gr]

Oh.. yes, what you say makes sense. I didn't expect the existing cause stuff would lead to an issue. I'll check if I can fix that and pr the fix against your branch, or merge your pr as is.

[ro0gr]

@simonihmig please take a look simonihmig#1

[simonihmig]

Thanks @ro0gr, merged it, so this PR should be good to go now I hope!

[ro0gr]

I'm just wondering if this was needed for some reason?

[simonihmig]

Yeah, right... this was needed to make TS understand that Error#cause is a thing. Was getting type errors without it when trying to access .cause.

[ro0gr]

Awesome. I've just left one minor question. Going to publish this tonight.

[simonihmig]

I've just left one minor question. Going to publish this tonight.

What is the question? 😄
This one? #640 (comment)

[ro0gr]

Hey, I've just released this https://github.com/san650/ember-cli-page-object/releases/tag/v2.3.0