imp: improved types and utils #1014

ssi02014 · 2024-11-07T09:15:26Z

Summary

@cure53 👋
I'm very happy to announce that DOMPurify now supports direct types.
I've improved some of the types and added const assertion for tighter control.

I made additional improvements to ts-doc.

ssi02014 · 2024-11-07T09:19:36Z

src/purify.ts

   */
-  const _removeAttribute = function (name, node) {
+  const _removeAttribute = function (name: string, element: Element): void {


I set the Node type to Element because it gives a type error, and rename it accordingly.

ssi02014 · 2024-11-07T09:20:10Z

src/utils.ts

+function unapply<T>(func: (thisArg: any, ...args: any[]) => T) {
+  return (thisArg: any, ...args: any[]): T => apply(func, thisArg, args);


I think it's much clearer than Function!

ssi02014 · 2024-11-07T09:21:12Z

src/utils.ts

@@ -137,7 +133,7 @@ function cleanArray(array: any[]): any[] {
 * @param object - The object to be cloned.
 * @returns A new object that copies the original.
 */
-function clone<T extends object>(object: T): T {
+function clone<T extends Record<string, any>>(object: T): T {


I unified the object types to the Record type.

cure53 · 2024-11-08T14:15:58Z

Thanks a ton 🙂

@reduckted and @aloisklink who have worked on the initial PRs - do you see any blockers or all good? We will likely release DOMPurify 3.2.0 soon in case all is fine.

ssi02014 · 2024-11-08T14:26:08Z

Let me know if you find anything wrong or if I missed anything 😃

reduckted

Looks OK. Just a lot of redundant JSDoc tags that are unnecessarily specifying the type.

reduckted · 2024-11-09T01:41:21Z

src/utils.ts

- * @param object - The object to look up the getter function in its prototype chain.
- * @param prop - The property name for which to find the getter function.
- * @returns The getter function found in the prototype chain or a fallback function.
+ * @param {T} object - The object to look up the getter function in its prototype chain.


The types do not need to be specified in the JSDoc comments. The type annotations define the type.

reduckted · 2024-11-09T01:42:00Z

src/utils.ts

@@ -134,10 +134,10 @@ function cleanArray(array: any[]): any[] {
 /**
 * Shallow clone an object
 *
- * @param object - The object to be cloned.
- * @returns A new object that copies the original.
+ * @param {T} object - The object to be cloned.


No need for the types in the JSDoc comments here.

reduckted · 2024-11-09T01:42:05Z

src/utils.ts

@@ -116,10 +116,10 @@ function addToSet(
 /**
 * Clean up an array to harden against CSPP
 *
- * @param array - The array to be cleaned.
- * @returns The cleaned version of the array
+ * @param {T[]} array - The array to be cleaned.


No need for the types in the JSDoc comments here.

reduckted · 2024-11-09T01:42:14Z

src/utils.ts

- * @param array - The array containing elements to be added to the set.
- * @param transformCaseFunc - An optional function to transform the case of each element before adding to the set.
- * @returns The modified set with added elements.
+ * @param {Record<string, any>} set - The set to which elements will be added.


No need for the types in the JSDoc comments here.

reduckted · 2024-11-09T01:42:19Z

src/utils.ts

 }

 /**
 * Creates a new function that constructs an instance of the given constructor function with the provided arguments.
 *
- * @param func - The constructor function to be wrapped and called.
- * @returns A new function that constructs an instance of the given constructor function with the provided arguments.
+ * @param {(...args: any[]) => T} func - The constructor function to be wrapped and called.


No need for the types in the JSDoc comments here.

reduckted · 2024-11-09T01:52:01Z

src/purify.ts

@@ -1810,17 +1827,17 @@ interface DOMPurify {
   * Remove a DOMPurify hook at a given entryPoint
   * (pops it from the stack of hooks if more are present)
   *
-   * @param  entryPoint entry point for the hook to remove
-   * @return removed(popped) hook
+   * @param {BasicHookName} entryPoint entry point for the hook to remove


The types can be removed from the JSDoc comments here.

reduckted · 2024-11-09T01:52:06Z

src/purify.ts

   */
  removeHook(entryPoint: BasicHookName): Hook | undefined;

  /**
   * Remove a DOMPurify hook at a given entryPoint
   * (pops it from the stack of hooks if more are present)
   *
-   * @param  entryPoint entry point for the hook to remove
-   * @return removed(popped) hook
+   * @param {'uponSanitizeElement'} entryPoint entry point for the hook to remove


The types can be removed from the JSDoc comments here.

reduckted · 2024-11-09T01:52:10Z

src/purify.ts

@@ -1830,8 +1847,8 @@ interface DOMPurify {
   * Remove a DOMPurify hook at a given entryPoint
   * (pops it from the stack of hooks if more are present)
   *
-   * @param  entryPoint entry point for the hook to remove
-   * @return removed(popped) hook
+   * @param {'uponSanitizeAttribute'} entryPoint entry point for the hook to remove


The types can be removed from the JSDoc comments here.

reduckted · 2024-11-09T01:52:16Z

src/purify.ts

@@ -1840,12 +1857,15 @@ interface DOMPurify {
  /**
   * Removes all DOMPurify hooks at a given entryPoint
   *
-   * @param entryPoint entry point for the hooks to remove
+   * @param {HookName} entryPoint entry point for the hooks to remove


The types can be removed from the JSDoc comments here.

reduckted · 2024-11-09T01:53:17Z

src/purify.ts

   */
  removeHooks(entryPoint: HookName): void;

  /**
   * Removes all DOMPurify hooks.
+   *
+   * @returns {void}


This JSDoc comment should be redundant since the return type annotation indicates the method returns void. @cure53 , do you want all functions to have an @returns JSDoc tag, even if they don't return a value?

ssi02014 · 2024-11-09T03:22:42Z

@reduckted @cure53 Yes, let's remove the unnecessarily specified type of JSDoc again!

I agree that after supporting typescript, we no longer need to specify types in the Jsdoc.

ssi02014 · 2024-11-09T04:22:15Z

I fixed it!

cure53 · 2024-11-09T11:15:42Z

Thanks a lot everyone 🙏

ssi02014 commented Nov 7, 2024

View reviewed changes

ssi02014 force-pushed the refactor/types branch 2 times, most recently from d748150 to 757d02c Compare November 7, 2024 09:31

imp: improved types and utils

fbd2ce8

ssi02014 force-pushed the refactor/types branch from 757d02c to fbd2ce8 Compare November 7, 2024 09:39

ssi02014 added 2 commits November 7, 2024 18:54

docs: improved ts-doc

5529184

docs: fixed ts-doc

0f7fc8a

reduckted suggested changes Nov 9, 2024

View reviewed changes

ssi02014 added 2 commits November 9, 2024 13:08

fix: remove types from jsdoc

4092e55

fix: remove types from jsdoc

fd7af7e

ssi02014 force-pushed the refactor/types branch from 96d598a to fd7af7e Compare November 9, 2024 04:12

fix: remove types from jsdoc

e75080a

reduckted approved these changes Nov 9, 2024

View reviewed changes

cure53 merged commit 0e54785 into cure53:main Nov 9, 2024
8 checks passed

ssi02014 deleted the refactor/types branch November 9, 2024 11:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

imp: improved types and utils #1014

imp: improved types and utils #1014

ssi02014 commented Nov 7, 2024 •

edited

Loading

ssi02014 Nov 7, 2024

ssi02014 Nov 7, 2024

ssi02014 Nov 7, 2024

cure53 commented Nov 8, 2024

ssi02014 commented Nov 8, 2024

reduckted left a comment

reduckted Nov 9, 2024

reduckted Nov 9, 2024

reduckted Nov 9, 2024

reduckted Nov 9, 2024

reduckted Nov 9, 2024

reduckted Nov 9, 2024

reduckted Nov 9, 2024

reduckted Nov 9, 2024

reduckted Nov 9, 2024

reduckted Nov 9, 2024

ssi02014 commented Nov 9, 2024 •

edited

Loading

ssi02014 commented Nov 9, 2024

cure53 commented Nov 9, 2024

		function unapply<T>(func: (thisArg: any, ...args: any[]) => T) {
		return (thisArg: any, ...args: any[]): T => apply(func, thisArg, args);

imp: improved types and utils #1014

imp: improved types and utils #1014

Conversation

ssi02014 commented Nov 7, 2024 • edited Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cure53 commented Nov 8, 2024

ssi02014 commented Nov 8, 2024

reduckted left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssi02014 commented Nov 9, 2024 • edited Loading

ssi02014 commented Nov 9, 2024

cure53 commented Nov 9, 2024

ssi02014 commented Nov 7, 2024 •

edited

Loading

ssi02014 commented Nov 9, 2024 •

edited

Loading