fix: replaced the old script that was blocking p5.sound.js multiple files' docs generation#1225
fix: replaced the old script that was blocking p5.sound.js multiple files' docs generation#1225kunstewi wants to merge 7 commits intoprocessing:2.0from
Conversation
There was a problem hiding this comment.
Thanks for the fix! I checked that it works locally, just minor note on p5.sound string -> const for readability
The old script used a for...in loop to iterate over the keys inside soundData.classes. In JavaScript, adding and deleting keys in an object while looping through it with a for...in loop is very risky. The JavaScript compiler may become confused. It could end up iterating over the newly added keys in an infinite loop, skipping existing keys, or generating errors.
Just to call this out, thanks for both the catch and the fix.
src/scripts/parsers/reference.ts
Outdated
| const classKeys = Object.keys(soundData.classes); | ||
| for (const key of classKeys) { | ||
| let newName = soundData.classes[key].name; | ||
| if (newName === 'p5.sound') { |
There was a problem hiding this comment.
Is there some way to factor out the hardcoded strings here into a const? Both 'p5.sound' and 'p5' prefixes are repeatedly used and there's some delicate logic in the whole setup (not this PR specifically, but overall)
There was a problem hiding this comment.
fixes #1226 and #72 (p5.sound.js)
@ksen0 @limzykenneth please take a look whenever you are free.
Thanks a lot for your time.
Old Script
Problems
In the p5.sound.js repository, global library functions like loadSound(), getAudioContext(), and userStartAudio() are documented with the tag @for p5.sound. YUIDoc reads this information and assigns the functions to the p5.sound class. The old script added p5. to everything without thinking, which meant it took those methods and assigned them to a class called p5.p5.sound that does not exist.
The old script mistakenly added p5. to the front of every class and item name, regardless of the situation. This works for a class named SoundFile, which becomes p5.SoundFile. However, if a developer specifically wrote @Class p5.SoundFile in the JSDoc comments, the old script would incorrectly change it to p5.p5.SoundFile.
The old script used a for...in loop to iterate over the keys inside soundData.classes. In JavaScript, adding and deleting keys in an object while looping through it with a for...in loop is very risky. The JavaScript compiler may become confused. It could end up iterating over the newly added keys in an infinite loop, skipping existing keys, or generating errors.
Updated Script
this updated script fixes these three critical bugs:
The new script specifically hunts for anything assigned to the p5.sound class and explicitly reassigns it to the p5 class. This correctly integrates them as global methods in the documentation (alongside things like createCanvas() and ellipse()) instead of breaking them.
The new script safely double-checks if the string already starts with "p5.". If it does, it skips it; if it doesn't, it adds it. This prevents double-prefixing.
The new script uses Object.keys() to grab a strict, immutable array of the original class names before the loop starts. It then safely loops over that array, ensuring that any modifications made to soundData.classes down below have no effect on the loop's logic.
Results
Previously no result for userStartAudio or any methods in Utils.js and also no loadSound method from SoundFile
After the change all methods are present in the reference docs