Skip to content
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

Add Type Declarations #1006

Merged
merged 10 commits into from
Oct 18, 2024
Merged

Add Type Declarations #1006

merged 10 commits into from
Oct 18, 2024

Conversation

reduckted
Copy link
Contributor

Summary

This pull request adds type declarations to this repository. This means that @types/dompurify will no longer be needed.

Fixes #1003

Background & Context

The type declarations provided by @types/dompurify did not quite match the code from this library. In particular, the way that this library is exported is different between CommonJS and ESM, meaning one set of type declarations is not sufficient. Additionally, the type declarations from @types/dompurify were missing some config properties and were also using the wrong DOM node types in the hook functions.

Changes

The type declarations were created by changing the src/*.js files to TypeScript files and using the TypeScript compiler to generate declaration files.

Unfortunately, due to the way that the DOMPurify object is defined, the type declarations could not be fully inferred from the DOMPurify definition. I think this is because it's sort of recursively defined - createDOMPurify defines the DOMPurify type which, in turn, is defined as a function that calls createDOMPurify. The TypeScript compiler was able to infer most of the structure of the DOMPurify type except for that function which was inferred as any. For example:

interface DOMPurify {
  (root: WindowLike): any;
  // ...etc...
}

To work around that problem, I've defined the DOMPurify interface manually. The good thing is, though, that because the DOMPurify object is annotated with the DOMPurify interface, it means that it must conform to that interface. Any changes to the interface without updating the definition will cause compilation errors, and vice versa.

The config is also manually defined, but similar to the DOMPurify interface, any new properties that are accessed on the cfg object will cause compilation errors unless the Config interface is also updated.

I've removed the type annotations from the JSDoc comments and moved them to be type annotations (and in some places removed them entirely, such as on DOMPurify's functions because the types come from the interface).

Note

I do want to stress that even though the files are now TypeScript, there's still a lot of untyped code (a lot of variables are any type). I did initially attempt to make sure everything was typed, but there were far too many changes and in some parts of the code it was outright impossible.

In terms of actual changes to the DOMPurify code, there were only two changes, and neither change should have any actual impact on consumers:

  1. forceKeepAttr was added to the initial hookEvent object instead of being added immediately before calling the hook.
  2. _executeHook was changed from a const to a function. This allowed overloads of that function to be defined, which added some type safety when executing hooks. This type safety ensures that if you call a hook with the wrong data parameter, a compilation error will occur. The benefit of this is that it ensures the hooks are always called with the type of data specified in the DOMPurify interface.

Rollup

Because the src/ files are now TypeScript, I had to add the rollup-plugin-typescript2 plugin to Rollup to process them. The only change this made to the output was some removal of whitespace.

To get a single type declaration file for CommonJS and a single file for ESM, I've used the rollup-plugin-dts. Without this, each file (attrs.ts, config.ts, etc.) would have a corresponding .d.ts file. The plugin merges those files into a single file, which is much cleaner.

To use that plugin, I had to update rollup from v2 to v3 (the latest is v4). The only impact this update had on the output was some reordering of object properties.

Type Declaration Fix

As I mentioned earlier, the way that DOMPurify is exported is different between CommonJS and ESM. Unfortunately, the type declarations do not reflect those differences, so a manual fix needs to be applied. For the CommonJS type declarations, the export default ... export (which would be equivalent to module.exports = { default: ... }) is replaced with export = ....

Tests

The tests are unchanged, and are still JavaScript files. The only change to any test-related files is in the Karma config, where it needed to change the file extension of the files that are used in the preprocessor, and how it uses the Rollup config - the Rollup config now exports an array, whereas it used to export an object (that object is now the first element in the array).

Differences to @types/dompurify

The type declarations are mostly the same as those in @types/dompurify, but there's a few key differences:

  • The callbacks for hooks now defines the type for this - the DOMPurify object.
  • The callbacks for hooks now define the first parameter as a Node, not an Element - this matches what DOMPurify can actually pass to a hook.
  • The overloads of sanitize are slightly different, but should result in the same return type based on the config properties.
  • removeHook now defines the hook as the return type instead of void.
  • removed now has a proper type instead of any[].
  • All properties on Config are now documented, and some missing properties have been added. I've taken most of this documentation from what was described in the README file or from pull requests where those properties were added.
  • Config.RETURN_DOM_IMPORT has been removed, because it was not used.

Are the types wrong?

I've confirmed that Are the types wrong? finds no problems with the type declarations. You can confirm this yourself using the CLI.

Install it if you haven't already done so:

npm i -g @arethetypeswrong/cli

Then build, pack and run the CLI:

npm run build
npm pack
attw .\dompurify-3.1.7.tgz

The output is:

dompurify v3.1.7

Build tools:
- typescript@^5.6.3
- rollup@^3.29.5

 No problems found 🌟


┌───────────────────┬─────────────┐
│                   │ "dompurify" │
├───────────────────┼─────────────┤
│ node10            │ 🟢          │
├───────────────────┼─────────────┤
│ node16 (from CJS) │ 🟢 (CJS)    │
├───────────────────┼─────────────┤
│ node16 (from ESM) │ 🟢 (ESM)    │
├───────────────────┼─────────────┤
│ bundler           │ 🟢          │
└───────────────────┴─────────────┘

New Version

I haven't bumped the version in this pull request, but I would recommend a major version bump. I think that will make it much easier to deprecate the @types/dompurify package.

Tasks

I've tried using these changes with:

  • A simple Node.js TypeScript project that uses {"module": "CommonJS", "moduleResolution": "Node10"}.
  • A simple Node.js TypeScript project that uses {"module": "NodeNext", "moduleResolution": "NodeNext"}.
  • A simple Node.js TypeScript project that uses {"module": "ESNext", "moduleResolution": "Bundler"}.
  • An Angular 18 project using the @angular-devkit/build-angular:browser builder.
  • An Angular 18 project using the @angular-devkit/build-angular:application builder.

In all cases, the code compiled and ran successfully.

I would love to get this tested in more real-world projects to help confirm that everything is working correctly.

You should be able to use it by installing from my fork:

npm i git+https://github.com/reduckted/DOMPurify.git#f487c8e5b9434e5db3b02a302559cf16f594b8cc

Dependencies

None.

@cure53 cure53 merged commit eb7b40d into cure53:main Oct 18, 2024
8 checks passed
@reduckted reduckted deleted the type-declarations branch October 18, 2024 22:09
@cure53
Copy link
Owner

cure53 commented Oct 20, 2024

That is one amazing PR, thank you very much :) We reviewed and merged and will do a new release soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Version 3.1.7 build error for Angular (likely other projects)
2 participants