Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the icon definitions script where icons were being accumulated into an ever-expanding array instead of a set, causing duplicate entries. The script has also been refactored from asynchronous to synchronous execution for debugging purposes.
- Fix icon accumulation logic by using Set instead of array concatenation
- Refactor from async/await to synchronous file operations
- Improve code organization and documentation
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // /*! iconNames: ['far fa-copy', 'fas fa-link', 'fab fa-github', 'fas fa-terminal', 'fal fa-external-link-alt'] */ | ||
|
|
||
| let contents = fs.readFileSync(iconDefsFile, 'utf8') | ||
| let firstLine = contents.substr(0, contents.indexOf("\n")); |
There was a problem hiding this comment.
Use substring() instead of the deprecated substr() method. Replace with contents.substring(0, contents.indexOf('\n')).
| let firstLine = contents.substr(0, contents.indexOf("\n")); | |
| let firstLine = contents.substring(0, contents.indexOf("\n")); |
| if (contents.includes(ICON_SIGNATURE_CS)) { | ||
| return contents.toString() | ||
| .match(ICON_RX) | ||
| .map((it) => it.substr(10)) |
There was a problem hiding this comment.
Use substring() instead of the deprecated substr() method. Replace with it.substring(10).
| .map((it) => it.substr(10)) | |
| .map((it) => it.substring(10)) |
|
|
||
| try { | ||
| const requiredIconNames = JSON.parse(firstLine.match(REQUIRED_ICON_NAMES_RX)[1].replace(/'/g, '"')) | ||
| console.log(requiredIconNames) |
There was a problem hiding this comment.
Remove this debug console.log statement as it appears to be leftover from debugging and should not be in production code.
| console.log(requiredIconNames) |
1 participant
Fix populate-icon-definitions by adding definitions to set rather than ever-expanding array.
This might not be the final version of work (as part of understanding and debugging the script, I changed it from async to sync... though on the other hand, it only takes < 10 seconds for the full Staging build, so it isn't unthinkable)