Add new sketch verifier to FES #7293
Conversation
…d variables and functions
package.json
Outdated
| @@ -23,6 +23,7 @@ | |||
| "version": "1.9.4", | |||
| "dependencies": { | |||
| "colorjs.io": "^0.5.2", | |||
| "espree": "^10.2.0", | |||
There was a problem hiding this comment.
Is the idea to go with Espree (and manual traverse of AST) or are there plans to further investigate alternatives?
There was a problem hiding this comment.
Based on my own experiments, Espree is consistently faster than Acorn, but I just dug around and it seems that now Espree is built on top of Acorn, and extends Acorn for additional functionalities. I feel like maybe given larger files / over time, Acorn might be more efficient? Since what we require from a parser is pretty basis and not an extended capability.
And I totally forgot that we could traverse in a more efficient way! Regardless of which parser we choose, we can leverage on a traversal utility library, likeacorn-walk for acorn or estraverse for espree.
Let me know what you think! I'm happy to do more formal benchmarking / experiments and have some sort of report / documentation before updating the implementation too.
cc: @davepagurek
There was a problem hiding this comment.
I think you can potentially do a bit of exploration for Espress vs Acorn, but the other things is whether using a linter is possible for this case or would manual AST traversal be the more realistic solution.
| */ | ||
| fn.fetchScript = async function (script) { | ||
| if (script.src) { | ||
| const contents = await fetch(script.src).then((res) => res.text()); |
There was a problem hiding this comment.
There's a chance this request will fail depending on access control policies of the server, we might want to wrap lines 16/17 in a try/catch. If there's an error, maybe we can just log the error and return an empty string?
There was a problem hiding this comment.
Yeah CORS is a potential problem here. I'm thinking would it be possible to catch the error, print a friendly message about it, then degrade into a lower version of the script content grabbing eg. the '' + window.setup technique used in the current sketch reader?
That way there is still some checks going on while advise on enabling CORS on the server will still be available.
There was a problem hiding this comment.
Added a TODO as a future item!
| */ | ||
| fn.getUserCode = async function () { | ||
| const scripts = document.querySelectorAll('script'); | ||
| const userCodeScript = scripts[scripts.length - 1]; |
There was a problem hiding this comment.
I wonder if there's a more robust way to get the user's script, since e.g. some sketches are spread across multiple files, or might import other addon libraries. We definitely don't need to be perfect here, but since multi-file sketches are pretty common on OpenProcessing, maybe we could do something like, filter out all the scripts that look like library code, e.g. because their src attribute matches /p5(\.\w+)\.js/, and then run the parser on the remaining scripts?
We could also exclude srcs that come from a known CDN domain like cdnjs.cloudflare.com or cdn.jsdelivr.net or unpkg.com. This is what OpenProcessing uses for its additional library toggles, so we could exclude more library code that way, although with some false negatives still.
There was a problem hiding this comment.
One way I was thinking about is to only grab from <script> tag that are in <body> which assumes that library import <script> tags are usually included in the <head> element while user code are included in the <body> tag, but that is mostly convention and not a guarantee so I'm not sure if it would be a good solution.
The other thing is that, if I remember correctly, if any <script> tag is using type=module they shouldn't be grabbed as well because these are scoped automatically.
| function traverse(node) { | ||
| const { type, declarations, id, init } = node; | ||
|
|
||
| switch (type) { |
There was a problem hiding this comment.
Definitely less common, but ClassDeclaration nodes might also be something to check
| const category = init && ['ArrowFunctionExpression', 'FunctionExpression'].includes(init.type) | ||
| ? 'functions' | ||
| : 'variables'; | ||
| userDefinitions[category].push(id.name); |
There was a problem hiding this comment.
It would be nice to give a line number too, although just thinking out loud, it looks like espree gives us a character range, so we'd have to count the number of \ns in preceding the start of the range of the node to get the line, which is a little annoying.
There was a problem hiding this comment.
Used acorn config to get locations for the lines
Addresses #7269
Changes:
sketch_verifier.jsfile to.../core/friendly_errors, this will replace currentsketch_reader.jssketch_verifier.jscan now read user's code and extract user-defined variables and fucntionsScreenshots of the change:
PR Checklist
npm run lintpasses