Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 theDOMPurify
type which, in turn, is defined as a function that callscreateDOMPurify
. The TypeScript compiler was able to infer most of the structure of theDOMPurify
type except for that function which was inferred asany
. For example: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 theDOMPurify
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 thecfg
object will cause compilation errors unless theConfig
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:
forceKeepAttr
was added to the initialhookEvent
object instead of being added immediately before calling the hook._executeHook
was changed from aconst
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 wrongdata
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 theDOMPurify
interface.Rollup
Because the
src/
files are now TypeScript, I had to add therollup-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, theexport default ...
export (which would be equivalent tomodule.exports = { default: ... }
) is replaced withexport = ...
.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:this
- theDOMPurify
object.Node
, not anElement
- this matches what DOMPurify can actually pass to a hook.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 ofvoid
.removed
now has a proper type instead ofany[]
.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:
Then build, pack and run the CLI:
npm run build npm pack attw .\dompurify-3.1.7.tgz
The output is:
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:
{"module": "CommonJS", "moduleResolution": "Node10"}
.{"module": "NodeNext", "moduleResolution": "NodeNext"}
.{"module": "ESNext", "moduleResolution": "Bundler"}
.@angular-devkit/build-angular:browser
builder.@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:
Dependencies
None.