Update sketch verifier to check for redefinitions and print friendly messages #7326
Conversation
davepagurek
left a comment
davepagurek
left a comment
There was a problem hiding this comment.
Nice work on this! The one thing I'd want to get a bit of clarification on is some of the false positive cases -- I wonder if that's from treating some p5 classes as globals when they no longer need to be?
| // reference on the p5.js website. | ||
| function generateFriendlyError(errorType, name, line) { | ||
| const url = `https://p5js.org/reference/#/p5/${name}`; | ||
| const message = `${errorType} "${name}" on line ${line} is being redeclared and conflicts with a p5.js ${errorType.toLowerCase()}. JavaScript does not declaring a ${errorType.toLowerCase()} more than once. p5.js reference: ${url}.`; |
There was a problem hiding this comment.
Looks like we maybe lost a verb in here:
| const message = `${errorType} "${name}" on line ${line} is being redeclared and conflicts with a p5.js ${errorType.toLowerCase()}. JavaScript does not declaring a ${errorType.toLowerCase()} more than once. p5.js reference: ${url}.`; | |
| const message = `${errorType} "${name}" on line ${line} is being redeclared and conflicts with a p5.js ${errorType.toLowerCase()}. JavaScript does not support declaring a ${errorType.toLowerCase()} more than once. p5.js reference: ${url}.`; |
There was a problem hiding this comment.
Also, I guess JavaScript does technically allow it in different scopes? We could consider saying "your code might not behave correctly" or "your code might behave unexpectedly" or something like that.
| 'toString', | ||
| 'print', | ||
| 'stop', | ||
| 'onended' |
There was a problem hiding this comment.
The functions meant to be redefined by the user make sense. Out of curiosity where do the other false positive entries come from? Is that from looking at classesWithGlobalFunctions?
limzykenneth
left a comment
limzykenneth
left a comment
There was a problem hiding this comment.
I think we should be able to leverage the AST more as it gives more context and information about where and what things are defined
| // in the p5.js library, either as a constant or as a function. | ||
| function checkForRedefinition(name, libValue, line, type) { | ||
| try { | ||
| const userValue = eval("name"); |
There was a problem hiding this comment.
I'm not sure what this line is meant to be doing but we want to avoid eval() as the bundler generally does work well with it.
|
I've also just checked the dependency sizes, it seems acorn and to a slightly lesser extend zod adds quite a bit to the bundle size. Are they importable as treeshakable modules so we don't include code we aren't using? |
|
@sproutleaf I kinda forgot about this one but is the changes in here still relevant for merge? |
3 participants
Addresses #7269
Changes:
checkForConstsAndFuncs()that checks for redefinitions and prints friendly error messagesPR Checklist