-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Add option to clear the console in Sandcastle #13013
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for the pull request, @jjspace! ✅ We can confirm we have a CLA on file for you. |
javagl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot really "review" the changes on the code level. (I had a short look, and ... "Yeah, 🤷♂️ looks OK...").
Beyond that, I spent ~1 hour trying to get the build running, and ~5 minutes testing it.
Building:
The npm run build-sandcastle causes this error for me:
[14:19:03] Using gulpfile C:\cesium\gulpfile.apps.js
[14:19:03] Starting 'buildSandcastle'...
[14:19:05] 'buildSandcastle' errored after 2.74 s
[14:19:05] Error: spawn C:\cesium\node_modules\typescript\bin\tsc ENOENT
at ChildProcess._handle.onexit (node:internal/child_process:285:19)
at onErrorNT (node:internal/child_process:483:16)
at process.processTicksAndRejections (node:internal/process/task_queues:90:21)
Figuring out that this came from
| const ls = spawn(binPath, ["-p", configPath]); |
Eventually, I changed that typescriptCompile promise to do
execSync("npx tsc -p C:/cesium/packages/sandcastle/templates/tsconfig.lib.json");
resolve(0);
But I should probably not create a PR for that 😶
Testing:
I ran a test for the base functionality, and it worked. And I ran a "stress test", trying to emulate the scenario where the "clear" functionality is most important, namely when some debug log is spamming the console with lots of stuff...
let counter = 0;
let delayMs = 5;
function spam() {
setTimeout(() => {
let s = " spam, ";
for (let i=0; i<100; i++) {
s += "spam, ";
}
console.log(counter + s + "with bacon and spam");
counter++;
spam();
}, delayMs);
}
spam();And despite the system being busy with generating that output, the "Clear console" button worked, and it cleared the console immediately, without noticable delays, and even when the console was already filled quite a bit.
Support multiple console arguments in sandcastle
|
Apologies for not clarifying merge order here, oversight on my part. Now that #13014 is merged this PR includes the changes for multiple arguments. Sorry if that makes the review more busy. Given that, @mzschwartz5 would you be able to look at this one too given your familiarity with my other pr? |
|
Yes but that will probably happen on Monday :) |
Description
As suggested in this comment there is now a button to clear the console for sandcastle.
I also hooked it up so that calling
console.clear()inside the sandcastle itself will clear the console mirror as well. I expect this could be useful to refresh the console while logging frequently during renders which some people may want to do.Issue number and link
No issue, part of #12894
Testing plan
console.clear()in the codeAuthor checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change