Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX beta] Improve error for improper invocations of link-to. #14554

Merged
merged 1 commit into from
Oct 30, 2016

Conversation

nathanhammond
Copy link
Member

Unfortunately, to get decent error messages, we need to do something like this. In some future state we should be able to use a "feature flag" which allows us to strip this without needing to call it twice.

if (isProdBuild()) {
  // Do the useful debug thing, probably including try/catch.
} else {
  // Do the performant thing.
}

* In some future state we should be able to use a "feature flag"
* which allows us to strip this without needing to call it twice.
*
* if (isProdBuild()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe either the truthy/falsey blocks should be flip/flopped or this conditional should be changed to isDebugBuild() (since we want to do the most performant thing in prod mode)...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely wrote the wrong thing. Adjusting.

@rwjblue
Copy link
Member

rwjblue commented Oct 30, 2016

This looks great, I have visions of threading through the callsite information so we can show template module name and line number info as well, but this is a great improvement already.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, definitely a better error message than the default one...

@nathanhammond
Copy link
Member Author

@rwjblue Note that the error, by including e.message, also includes the name of the segment that you didn't pass in via tildeio/route-recognizer#116. 😄

@nathanhammond
Copy link
Member Author

K, we're good here.

@rwjblue rwjblue merged commit 1699a4f into emberjs:master Oct 30, 2016
@rwjblue
Copy link
Member

rwjblue commented Oct 30, 2016

Awesome, thank you @nathanhammond!

@nathanhammond nathanhammond deleted the improve-link-to-error branch October 30, 2016 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants