-
Notifications
You must be signed in to change notification settings - Fork 75
feat: Integrate JSDoc output into vitepress #1240
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
…nto jsdoc-to-markdow
|
Gianluca Beil seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
| } | ||
|
|
||
| function stripHtmlComments(str) { | ||
| return str.replace(/<!--[\s\S]*?-->/g, ""); |
Check failure
Code scanning / CodeQL
Incomplete multi-character sanitization High documentation
<!--
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
To fully eliminate all HTML comments—taking into account the incomplete removal bug—the stripHtmlComments function needs to apply the replacement repeatedly until no more matches are found. This can be accomplished by wrapping the existing .replace() call in a loop, comparing the previous value to the newly replaced value, and continuing until the string stabilizes. This ensures any newly formed or revealed matches are also sanitized. This change is localized to the stripHtmlComments function—for clarity and safety, no other code needs to be changed. No external dependencies are required for this fix, and the function interface remains the same.
-
Copy modified lines R190-R195
| @@ -187,7 +187,12 @@ | ||
| } | ||
|
|
||
| function stripHtmlComments(str) { | ||
| return str.replace(/<!--[\s\S]*?-->/g, ""); | ||
| let previous; | ||
| do { | ||
| previous = str; | ||
| str = str.replace(/<!--[\s\S]*?-->/g, ""); | ||
| } while (str !== previous); | ||
| return str; | ||
| } | ||
|
|
||
| console.log("Conversion done"); |
# Conflicts: # internal/documentation/scripts/downloadPackages.sh
|
|
||
| // Recursively handle the inner type | ||
| if (isComplexTypeExpression(innerType)) { | ||
| result += linkComplexType(innerType, null, classString.replace(' class="', '').replace('"', '')); |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High documentation
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
In general, to fix this kind of problem you should either use a proper sanitization/escaping library or, if you’re just doing simple character stripping, use a regular expression with the global (g) flag so that all occurrences are replaced, not just the first. When removing quotation marks or similar characters, make sure to handle all instances so partially-sanitized strings cannot cause malformed output.
In this concrete case, the intent on line 748 is to turn something like classString (probably class="SomeClass") into just the inner value (SomeClass) for use as a class attribute when generating links for complex types. The current code does this by first removing the substring class=" and then removing only the first remaining " character. To make this robust, we should (a) remove the leading class=" as now, and then (b) remove all remaining double quotes using a regex with the global flag. This preserves existing behavior for simple cases, but correctly handles any classString with more than one double quote.
Concretely, in internal/documentation/jsdoc/docdash/publish.js, within linkGenericType, change line 748 so that the second .replace uses a global regex: .replace(/"/g, '') instead of .replace('"', ''). No new imports or helper methods are required.
-
Copy modified line R748
| @@ -745,7 +745,7 @@ | ||
|
|
||
| // Recursively handle the inner type | ||
| if (isComplexTypeExpression(innerType)) { | ||
| result += linkComplexType(innerType, null, classString.replace(' class="', '').replace('"', '')); | ||
| result += linkComplexType(innerType, null, classString.replace(' class="', '').replace(/"/g, '')); | ||
| } else { | ||
| let innerUrl = helper.longnameToUrl[innerType]; | ||
| if (innerUrl) { |
| } | ||
| } | ||
| }, | ||
| "githubSourceBaseUrl": "https://github.com/UI5/cli/blob/main/pacakges/" |
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.
| "githubSourceBaseUrl": "https://github.com/UI5/cli/blob/main/pacakges/" | |
| "githubSourceBaseUrl": "https://github.com/UI5/cli/blob/main/packages/" |
| @@ -1,4 +1,4 @@ | |||
| (function(window) { | |||
| nbpm(function(window) { | |||
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.
?
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.
?
d3xter666
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.
And one general comment:
We shall not modify the content within packages/ folder, nor delete anything from there. That will impact the UI5 CLI functionality, tests and docs
| if (process.argv[2] == "gh-pages") { | ||
| // Read package.json of packages/builder | ||
| const builderJson = JSON.parse(fs.readFileSync("tmp/packages/@ui5/builder/package.json")); | ||
| packageTagName = `https://github.com/UI5/cli/blob/cli-v${builderJson["version"]}/packages/`; |
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.
This link is invalid, because the previous versions of UI5 CLI are not within a monorepo, but separate packages and they have separate repositories. There is no packages folder, except for the main branch
| for (const file of fs.readdirSync(outputDirectory)) { | ||
| fs.rmSync(path.join(outputDirectory, file)); | ||
| } | ||
| } |
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.
You can use the rimraf package and clean the whole dir
| } | ||
|
|
||
| async function checkDeadlinks(link, sourcePath) { | ||
| if ((await fetch(link)).status != 200) { |
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.
It should not be within the 4xx range. Please take a look here: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status
| @@ -0,0 +1,92 @@ | |||
| ## Modified by SAP | |||
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.
Why is this package copied here, but not used as dependency to the documentation?
| * <pre> | ||
| * this["name"] = name; | ||
| * </pre> |
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.
This is how our current JSDoc works. Can we handle this in the transofrmation?
I know that if we leave it like that here, the vitepress build might fail
JIRA: CPOUI5FOUNDATION-904