Skip to content

[WIP] display the script error caused by external library #579

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

Merged
merged 3 commits into from
Mar 2, 2018

Conversation

shinytang6
Copy link
Member

@shinytang6 shinytang6 commented Feb 28, 2018

Before your pull request is reviewed and merged, make sure you

  • there are no linting errors -- npm run lint
  • code is in uniquely-named feature branch, and has been rebased on top of latest master. If you're asked to make more changes make sure you rebase onto master then too!
  • pull request is descriptively named and links to an issue number, i.e. Fixes #123

Thank you!

@shinytang6
Copy link
Member Author

partly relates to #468 :)
fixes e.g.

let txt;

function setup() { 
  createCanvas(400, 400);
  text(txt,100,100);
} 
function setup() { 
  createCanvas(400, 400);
} 

function draw() { 
  background(220);
  rotateZ(100);
}

Still need to know more about how to display lineNumber when the error caused by external library.
Currently it only shows the error line in e.g p5.min.js :)

@catarak
Copy link
Member

catarak commented Feb 28, 2018

this is interesting and i'm not sure how to handle it. the issue is that the user is using the library improperly. i think the ideal case is that we would get the full stack trace of the error, figure out the line that is in one of the files in sketch itself, and put the error on that line, and show the full stack trace in the console. can you get the whole stack trace? there's a script called hijackConsole.js that sends the console messages through a message channel to the web editor console.

@catarak catarak changed the title display the script error caused by external library [WIP] display the script error caused by external library Feb 28, 2018
@shinytang6
Copy link
Member Author

The first one seems to be working properly now:)

function setup() { 
  createCanvas(400, 400);
} 

function draw() { 
  background(220);
  rotateZ(100);
}

this one doesn't show whole stack trace, only reports Uncaught not supported in p2d. Please use webgl mode, l think it is relevant to the p5.js's console?

@catarak
Copy link
Member

catarak commented Mar 1, 2018

cool, i like where this is going! i wonder if it would make sense to send the whole stack back to the in-window console? is it too confusing for a coding newbie?
screen shot 2018-03-01 at 1 43 31 pm

vs.

screen shot 2018-03-01 at 1 43 07 pm

is there potentially a happy medium? as someone with coding experience i found the error confusing because i was like, "toString isn't being called on this line," and i think it's important to say that the error is being thrown in another file, but is a result of the user not using the library correctly, and therefore there still is in error. thoughts @lmccart & @shiffman?

i also just changed the p5 version to the non-minified version to see if the friendly errors system would catch it, and it doesn't.

@shiffman
Copy link
Member

shiffman commented Mar 1, 2018

I think the ideal outcome would be for the error to read something like:

p5 cannot render undefined text. sketch: line 5

So maybe this is a matter of adding this particular scenario to the friendly errors system?

If rewriting the error is not an option I think I would stick with the single error over the full stack trace for the web editor? We could reconsider this based on any future work on the console itself #7 -- for example, maybe the full stack trace could "unfold"? But my instinct says that the full stack trace is more scary than helpful, and it would be available in the chrome developer tools should someone want to look.

@catarak
Copy link
Member

catarak commented Mar 1, 2018

makes sense to me!

@shinytang6
Copy link
Member Author

shinytang6 commented Mar 2, 2018

@catarak @shiffman so what is the verdict on this one currently:)

@shiffman
Copy link
Member

shiffman commented Mar 2, 2018

For now let's go with "Uncaught TypeError: Cannot read property 'toString' of undefined (sketch: Line 5)". I can add an issue regarding a friendly error for this separately in the p5.js repo.

@shinytang6
Copy link
Member Author

That will be fine 😄

@catarak
Copy link
Member

catarak commented Mar 2, 2018

cool, then this PR is good to be merged.

@catarak catarak merged commit 4e5c585 into processing:master Mar 2, 2018
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.

3 participants