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

How to import DOMPurify as an ECMAScript module from TypeScript? #705

Closed
amdw opened this issue Aug 3, 2022 · 13 comments
Closed

How to import DOMPurify as an ECMAScript module from TypeScript? #705

amdw opened this issue Aug 3, 2022 · 13 comments

Comments

@amdw
Copy link

amdw commented Aug 3, 2022

Some JavaScript environments discourage the use of CommonJS modules, because they make it harder for bundlers and minifiers to optimise an application. For example, here is the advice from Angular:

https://angular.io/guide/build#configuring-commonjs-dependencies

I've been trying to import DOMPurify from a TypeScript application using ECMAScript module syntax without depending on a CommonJS module, but I can't find a way to do it.

If I just follow the documentation and say:

import DOMPurify from 'dompurify';

Then it works, provided I set allowSyntheticDefaultImports to true in my tsconfig.json. But this causes a dependency on the CommonJS module, which is what I'm trying to avoid. I think this is because the main attribute of DOMPurify's package.json points to the CommonJS module.

I can see the DOMPurify build does include a purify.es.js file which appears to be an ECMAScript module. So if we could just get TypeScript to import DOMPurify from that file instead of the CommonJS file, things would probably work. I tried doing this:

import DOMPurify from 'dompurify/dist/purify.es.js';

And it actually seems to work, in that it does seem to cause the TypeScript compiler to look for DOMPurify in that file. However, the problem is that the TypeScript compiler cannot find the type declarations, even though @types/dompurify is installed in my project:

error TS7016: Could not find a declaration file for module 'dompurify/dist/purify.es.js'. '<snip>/node_modules/dompurify/dist/purify.es.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/dompurify` if it exists or add a new declaration (.d.ts) file containing `declare module 'dompurify/dist/purify.es.js';`

So the problem now is that TypeScript doesn't know where to look for the type definitions for this module, now I'm not importing it using the standard dompurify name.

What I'm doing at the moment (reference) is,

import * as DOMPurify from 'dompurify';

Which allows me to import the CommonJS module without allowSyntheticDefaultImports. Then, I'm adding dompurify to allowedCommonJsDependencies in my angular.json to suppress the compiler warning that I'm depending on a CommonJS module. But that's not very satisfactory.

I'm not sure if this is a problem with DOMPurify or @types/dompurify, or if there is some kind of TypeScript compiler option or import syntax or something I can use that will make everything just work with the ECMAScript module. But I spent considerable time today poring through TypeScript's modules and module resolution documentation and I couldn't find anything.

If someone cleverer than me can find a way to import DOMPurify as an ECMAScript module from TypeScript, it would be great to add this to the DOMPurify README to help users in this situation. If this is in fact impossible then it would be great to consider changes to DOMPurify and/or @types/dompurify to make it possible.

Thanks very much for any help!

@cure53
Copy link
Owner

cure53 commented Aug 3, 2022

+1 If anyone has a smart way to solve this, I'd e more than happy to review and accept a PR.

@cure53
Copy link
Owner

cure53 commented Aug 3, 2022

cc @tdeekens who might have more experience on the topic or know who to ask about this :)

@amdw
Copy link
Author

amdw commented Aug 3, 2022

For what it's worth, I tried modifying DOMPurify's package.json locally in my node_modules folder so main points to dist/purify.es.js instead of dist/purify.cjs.js, and I still get the compiler warning:

Warning: <snip>/markdownify.pipe.ts depends on 'dompurify'. CommonJS or AMD dependencies can cause optimization bailouts.

If I turn on traceResolution in tsconfig.json, I see the following:

'package.json' has 'main' field 'dist/purify.es.js' that references '<snip>/node_modules/dompurify/dist/purify.es.js'.
File '<snip>/node_modules/dompurify/dist/purify.es.js' exist - use it as a name resolution result.
File '<snip>/node_modules/dompurify/dist/purify.es.js' has an unsupported extension, so skipping it.

and then some stuff about resolving @types/dompurify. This leads me to think @types/dompurify is a problem as well.

As you can tell, I'm way out of my depth here, but I'd guess it's probably possible to achieve what I want by both changing main in package.json to point to the ECMAScript module file and changing index.d.ts so it declares the types in an ECMAScript-module fashion as well.

The problem is that may be a breaking change. Hopefully someone who knows what they're talking about can confirm whether I'm right and whether there's a way to have the best of both worlds here.

@cure53
Copy link
Owner

cure53 commented Aug 4, 2022

@amdw Thanks for looking into this! I'd be happy to change package.json if that does the trick on our end. What would have to be changed here, on this end?

@cure53
Copy link
Owner

cure53 commented Aug 9, 2022

@amdw I have been checking another thread and believe that this might be a first step towards a working fix:

#691 (comment)

I ran the command (slightly modified) and generated the type definitions:

npx -p typescript tsc dist/*.js --declaration --allowJs --emitDeclarationOnly --outDir types

Does that change anything from your end when done locally? The types folder is being created, filled up with the type definitions generated from the dist folder.

@cure53
Copy link
Owner

cure53 commented Aug 10, 2022

Just a heads-up, we updated the README. Maybe this helps solving some of the issues?

https://github.com/cure53/DOMPurify#running-dompurify-on-the-server

amdw added a commit to google/peoplemath that referenced this issue Aug 11, 2022
@amdw
Copy link
Author

amdw commented Aug 11, 2022

Many thanks for trying here and apologies for the slow reply!

I tried generating the types using the command in your comment. For me they don't get resolved unless I put them in the dist folder - putting them in a separate types folder doesn't work, I think because that folder isn't in the places TypeScript looks during module resolution - but even then, I still get a build error saying I'm depending on a CommonJS module.

I pushed my code to a branch called dpmodule:

google/peoplemath@master...dpmodule

From the build logs I can see the purify.es.js file is being resolved, and so is the purify.es.d.ts file generated by the command you suggested. However the compiler still seems to think there is a dependency on a CommonJS module somehow.

'package.json' has 'main' field 'dist/purify.es.js' that references '<snip>/dev/peoplemath/node_modules/dompurify/dist/purify.es.js'.
File '<snip>/dev/peoplemath/node_modules/dompurify/dist/purify.es.js' exist - use it as a name resolution result.
File '<snip>/dev/peoplemath/node_modules/dompurify/dist/purify.es.js' has an unsupported extension, so skipping it.
Loading module as file / folder, candidate module location '<snip>/dev/peoplemath/node_modules/dompurify/dist/purify.es.js', target file type 'TypeScript'.
...
Resolving real path for '<snip>/dev/peoplemath/node_modules/dompurify/dist/purify.es.d.ts', result '<snip>/dev/peoplemath/node_modules/dompurify/dist/purify.es.d.ts'.
======== Module name 'dompurify' was successfully resolved to '<snip>/dev/peoplemath/node_modules/dompurify/dist/purify.es.d.ts' with Package ID 'dompurify/dist/purify.es.d.ts@2.3.10'. ========
...
Warning: <snip>/dev/peoplemath/src/app/markdown/markdownify.pipe.ts depends on 'dompurify'. CommonJS or AMD dependencies can cause optimization bailouts.
For more info see: https://angular.io/guide/build#configuring-commonjs-dependencies

To reproduce this:

git clone https://github.com/google/peoplemath.git --branch dpmodule
cd peoplemath
npm install
./dompurify_repro.sh
npx ng build --configuration production | grep dompurify

I wish I could tell you why this happens and how to fix it, but unfortunately I have no idea. That's as far as I've got, sorry.

@cure53
Copy link
Owner

cure53 commented Aug 13, 2022

@amdw thanks a ton for setting up this repo, this is amazing and for the first time allows me to actually debug that problem and understand what is happening :D

I think I already came close to a solution, will test some things and get back to you in this thread asap.

@cure53
Copy link
Owner

cure53 commented Aug 13, 2022

Alright, this should work. Locally, your repo produces an error free build when I do the following changes:

  • Place type definitions for purify.cjs.js and purify.es.js in dist folder, latest commit in main does that
  • Change back the code in markdownify.pipe.ts#18 to the following import * as DOMPurify from 'dompurify'

This allows me to build error free and should solve your problem without making any other changes. Is this a valid solution?

Alternatively, you can leave the code in the markdownify.pipe.ts as is and make sure that main points to the module build and not the Common JS build, then it also works without further ado.

@cure53
Copy link
Owner

cure53 commented Aug 23, 2022

I think it should be fair to mark this as done :) Please reopen in case anything still doesn't work.

@cure53 cure53 closed this as completed Aug 23, 2022
@amdw
Copy link
Author

amdw commented Aug 30, 2022

Apologies for my very slow response here. Thank you very much for the efforts you have made to solve this.

Unfortunately I am not able to reproduce the fix.

Firstly, it appears that the additions you made to dist in commit 4f62dcd were subsequently reverted in commit 652d200.

I tried the following to copy the files into dist from the earlier commit:

git clone https://github.com/cure53/DOMPurify.git
cd DOMPurify
git checkout 4f62dcd63cdc19167261cb980cdeba9b8ae2d2ca
cd ../peoplemath
cp ../DOMPurify/dist/purify.*.d.ts node_modules/dompurify/dist/
npx ng build --configuration production

However I still get the warning:

Warning: /usr/local/google/home/amedworth/dev/peoplemath/src/app/markdown/markdownify.pipe.ts depends on 'dompurify'. CommonJS or AMD dependencies can cause optimization bailouts.
For more info see: https://angular.io/guide/build#configuring-commonjs-dependencies

The only change I made to my project was to remove the allowedCommonJsDependencies from my angular.json. I created a branch dprepro with this change for convenience:

https://github.com/google/peoplemath/compare/dprepro

Cloning the repro at this branch and doing the above should make it possible to reproduce this warning.

I'm also slightly confused by this:

Alternatively, you can leave the code in the markdownify.pipe.ts as is and make sure that main points to the module build and not the Common JS build, then it also works without further ado.

This change to the main element of dompurify's package.json was exactly what my patch script did in the dpmodule branch, and it didn't help.

https://github.com/google/peoplemath/compare/dpmodule

Since I can't reproduce the fix and one of the key commits has been reverted, would it be possible to reopen this issue? Many thanks!

@cure53
Copy link
Owner

cure53 commented Aug 30, 2022

Hi @amdw - yes indeed - we reverted all changes.

The fixes worked for some folks (#691, #705 (comment)) and caused trouble for others (#712, #713), so we decided to roll back all changes and to leave the typing to DefinitelyTyped and all build issues to the folks who maintain the respective builds.

Reopening the issue might not make sense as I don't think this is a bug on our end and don't have resources available to invest more time into this issue, sorry.

If you have any idea how our project can do things better, please do let me know, happy to look into it. But I don't think the error is on our end and I am hesitant to experiment around any further as it might cause bugs for people who are fine right now.

@onpaws
Copy link

onpaws commented Apr 4, 2023

In case anyone else is tempted by isomorphic-dompurify FYI it will not offer an ESM-compatible way to avoid the Ivy compiler "optimization bailouts" message as of the time of writing.

Warning: /Users/paws/Developer/+GRP/GRP-Web/src/app/popups/preview-html-code/preview-html-code.component.ts depends on 'isomorphic-dompurify'. CommonJS or AMD dependencies can cause optimization bailouts.
For more info see: https://angular.io/guide/build#configuring-commonjs-dependencies

I hope something will develop here🤞

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

No branches or pull requests

3 participants