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

core(lantern): drop node id from error message #6774

Merged
merged 2 commits into from
Dec 11, 2018
Merged

core(lantern): drop node id from error message #6774

merged 2 commits into from
Dec 11, 2018

Conversation

paulirish
Copy link
Member

In LR we collect and report all our errors. This one causes our metrics to get a little funky since it changes every time.
Does this work for you, @patrickhulce.

Also i imagine you want a few URLs that can reproduce this?

b/120845363

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

WFM 👍

Yeah a repro case of this would be awesome :)

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

is the id ever helpful when debugging? If not, seems fine to drop it. If so, maybe make an LHError and stick it on a property?

@patrickhulce
Copy link
Collaborator

is the id ever helpful when debugging? If not, seems fine to drop it.

It isn't as an isolated aggregate error message.

It's really useful when you're logging other parts of the graph too like I was when working on lantern and can look at the nodes to see which it is, but at that point, could just add back in a log statement. Might be nice to have some extra use of debug where useful log statements can be added that aren't run by default and can be turned back on when re-entering a specific domain. I fear any lantern stuff at this point would be too chatty for global verbose.

@@ -209,7 +209,7 @@ class BaseNode {
}
});

if (!idToNodeMap.has(this.id)) throw new Error(`Cloned graph missing node ${this.id}`);
if (!idToNodeMap.has(this.id)) throw new Error(`Cloned graph missing node`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

smallest nit in the world of nits....BACKTICK -> COMMA TO THE TOP since there's no more interpolation

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops. I figured that edit would show up as a suggestion. I just changed it for you :)

@paulirish paulirish merged commit bebcd6f into master Dec 11, 2018
@paulirish paulirish deleted the nodeid branch December 11, 2018 23:38
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.

5 participants