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

3.2.0 seems to cause a build error in Angular/React: An export assignment cannot be used in a module with other exported elements. #1018

Closed
ghiscoding opened this issue Nov 12, 2024 · 19 comments · Fixed by #1022

Comments

@ghiscoding
Copy link
Contributor

ghiscoding commented Nov 12, 2024

Background & Context

I tried to upgrade to latest v3.2.0 that drops the need for the @types/dompurify which is awesome, however my Angular build is now failing with this error (you can see the error in my CI workflow failure)

Error: node_modules/dompurify/dist/purify.cjs.d.ts:401:1 - error TS2309: An export assignment cannot be used in a module with other exported elements.

401 export = _default;

If we browse to the CJS dist bundle from unpkg, we can see that it's the last line of the file export that it doesn't seem to like. I don't fully understand the problem but it might be due to trying to export DOMPurify twice? Though again I'm not entirely sure, see this Stack Overflow answer for that error

Bug

Also another possible issue that I found on top of what is described above, is that I was a bit surprised that it picked up the CJS (CommonJS) build instead of the expect ESM dist. Taking a look at the package.json, it is most probably because the CJS require is the first export to appear, however it should really be the import that comes first and make the require a fallback (2nd) which is the inverse of what is currently set. Note that I could provide a PR for the correct order but I'm not really that will fix the 1st error above

DOMPurify/package.json

Lines 29 to 38 in 2cf6e25

"exports": {
".": {
"require": {
"types": "./dist/purify.cjs.d.ts",
"default": "./dist/purify.cjs.js"
},
"import": {
"types": "./dist/purify.es.d.mts",
"default": "./dist/purify.es.mjs"
}

We can see the correct order in NodeJS conditional exports

Within the "exports" object, key order is significant. During condition matching, earlier entries have higher priority and take precedence over later entries. The general rule is that conditions should be from most specific to least specific in object order.

Input

n/a

Given output

n/a

Expected output

no build error

Feature

n/a

@sumitramanga
Copy link

sumitramanga commented Nov 13, 2024

I have this problem too but in a React app 🤔 I can confirm I don't have this issue on v3.1.7

@aky-wtag
Copy link

same issue here

@ghiscoding
Copy link
Contributor Author

ghiscoding commented Nov 13, 2024

a bit more info on this, if we modify the file node_modules/dompurify/dist/purify.cjs.d.ts with the following (which becomes the same as the ESM d.ts)

+ export { type Config, type Hook, type HookName, type RemovedAttribute, type RemovedElement, type UponSanitizeAttributeHook, type UponSanitizeAttributeHookEvent, type UponSanitizeElementHook, type UponSanitizeElementHookEvent, type WindowLike, type DOMPurify };
- export { type Config, type Hook, type HookName, type RemovedAttribute, type RemovedElement, type UponSanitizeAttributeHook, type UponSanitizeAttributeHookEvent, type UponSanitizeElementHook, type UponSanitizeElementHookEvent, type WindowLike, type DOMPurify, _default as default };

- export = _default;

then it works.

However, this change was actually modified by this scripts/fix-cjs-types.js script which is basically what seems to break the types.

// DOMPurify is exported as a default export, but rollup changes
// it to be assigned to `module.exports`. We need to change the
// type declarations to match what rollup changed it to. Remove
// the "default" export from the `export { ... }` statement.
let fixed = types.replace(', _default as default', '');

in short, it looks like the scripts/fix-cjs-types.js is causing the issue, so the question would be why was it implemented that way and is it really needed? From what I've always used in the past, it's typically enough to simply copy the d.ts to d.cts (with the exact same content). So let's try these exact steps, copy the dist/purify.es.d.mts and rename it as dist/purify.es.d.cts and then update the package.json CJS types references... yup that also work on my end.

"types": "./dist/purify.es.d.cts",
  "exports": {
    ".": {
      "import": {
        "types": "./dist/purify.es.d.mts",
        "default": "./dist/purify.es.mjs"
      },
      "require": {
        "types": "./dist/purify.es.d.cts",
        "default": "./dist/purify.cjs.js"
      }
    }
  },

So is the scripts/fix-cjs-types.js script really necessary? It seems to be the cause of all the problems here

@ghiscoding ghiscoding changed the title 3.2.0 seems to cause a build error in Angular: An export assignment cannot be used in a module with other exported elements. 3.2.0 seems to cause a build error in Angular/React: An export assignment cannot be used in a module with other exported elements. Nov 13, 2024
@cure53
Copy link
Owner

cure53 commented Nov 13, 2024

Good question, cc @reduckted and @ssi02014 maybe? 🙂

@reduckted
Copy link
Contributor

So is the scripts/fix-cjs-types.js script really necessary? It seems to be the cause of all the problems here

It's necessary to make the types correct according to arethetypeswrong.

With that script:

dompurify v3.2.0

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

 No problems found 🌟


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

Without that script:

dompurify v3.2.0

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

❗️ The resolved types use export default where the JavaScript file appears to use module.exports =. This will cause TypeScript under the node16 module mode to think an extra .default property 
access is required, but that will likely fail at runtime. These types should use export = instead of export default. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseExportDefault.md

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

Whether there's another way to fix it, I don't know. 😕

@reduckted
Copy link
Contributor

@ghiscoding, @sumitramanga, @aky-wtag Can you provide a reproduction of the error? I've created an Angular application and using dompurify works correctly, so there must be something that is different in your configuration.

@ghiscoding
Copy link
Contributor Author

ghiscoding commented Nov 13, 2024

@reduckted I got the problem on my open source project Angular-Slickgrid, I rolled back the change but basically just cloning the project and updating the package.json to 3.2 again, then run dev scripts will reproduce the error.

Whether there's another way to fix it, I don't know. 😕

Like I said in my previous comment, simply copying the d.ts file to d.cts is typically enough for "Are the Types Wrong" (ATTW) to work correctly and that is literally what I do in couple of my other open source libs and "ATTW" is totally fine. I can test it tonight, but basically I use this little script that runs as a postbuild script. I can test it later after work if you want, however I did test the rename of d.ts file to d.cts and it was working, but I didn't think to test ATTW. I'll test it later after work, I'm not sure if the reason it didn't work is because you use cjs.d.ts instead of d.cts, .cts makes it explicitly detect as a CJS file.

// clone-dts.mjs
import { readFileSync, writeFileSync } from 'node:fs';
writeFileSync('dist/index.d.cts', readFileSync('dist/index.d.ts'));

and it produces this on ATTW
image

@asamuzaK
Copy link
Contributor

Hi, I got the following errors with DOMPurify 3.2.0

>npx tsc

node_modules/dompurify/dist/purify.es.d.mts:170:28 - error TS2304: Cannot find name 'TrustedTypePolicy'.

170     TRUSTED_TYPES_POLICY?: TrustedTypePolicy | undefined;
                               ~~~~~~~~~~~~~~~~~

node_modules/dompurify/dist/purify.es.d.mts:245:9 - error TS2304: Cannot find name 'TrustedHTML'.

245     }): TrustedHTML;
            ~~~~~~~~~~~

node_modules/dompurify/dist/purify.es.d.mts:396:34 - error TS2339: Property 'trustedTypes' does not exist on type 'Window & typeof globalThis'.

396     trustedTypes?: typeof window.trustedTypes;
                                     ~~~~~~~~~~~~

src/mjs/dompurify.js:5:1 - error TS9006: Declaration emit for this file requires using private name 'DOMPurify' from module '"C:/Users/XXX/Documents/YYY/node_modules/dompurify/dist/purify.es"'. An explicit type annotation may unblock declaration emit.

5 import DOMPurify from 'dompurify';
  ~~~~~~

Found 4 errors in 2 files.

@ghiscoding
Copy link
Contributor Author

@asamuzaK that's a different and probably separate error, you're probably missing the dev dependencies @types/node and/or @types/trusted-types, 1 of these 2 should work or both of them, anyway give it a try

@asamuzaK
Copy link
Contributor

@ghiscoding
Ah, I didn't know @types/trusted-types is required.
Works now, thanks.

@sumitramanga
Copy link

sumitramanga commented Nov 13, 2024

@reduckted Thanks Dave! The following is my build log. I see that I need @types/trusted-types now too according to the above but what about the other errors

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(130,25):
00:54:16   TS2304: Cannot find name 'DOMParserSupportedType'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts
00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(170,28):
00:54:16   TS2304: Cannot find name 'TrustedTypePolicy'

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(245,9):
00:54:16   TS2304: Cannot find name 'TrustedHTML'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(395,37):
00:54:16   TS2339: Property 'NamedNodeMap' does not exist on type 'Window'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(396,34):
00:54:16   TS2339: Property 'trustedTypes' does not exist on type 'Window'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,10):
00:54:16   TS2300: Duplicate identifier 'type'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,10):
00:54:16   TS2304: Cannot find name 'type'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,15):
00:54:16   TS1005: ',' expected.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,23):
00:54:16   TS2300: Duplicate identifier 'type'.
00:54:16   
00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,23):
00:54:16   TS2304: Cannot find name 'type'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,28):
00:54:16   TS1005: ',' expected.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,34):
00:54:16   TS2300: Duplicate identifier 'type'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,34):
00:54:16   TS2304: Cannot find name 'type'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,39):
00:54:16   TS1005: ',' expected.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,49):
00:54:16   TS2300: Duplicate identifier 'type'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,49):
00:54:16   TS2304: Cannot find name 'type'.

00:54:16   ERROR in C:/app/dompurify/dist/purify.cjs.d.ts(399,54):
00:54:16   TS1005: ',' expected.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,72):
00:54:16   TS2300: Duplicate identifier 'type'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,72):
00:54:16   TS2304: Cannot find name 'type'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,77):
00:54:16   TS1005: ',' expected.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,93):
00:54:16   TS2300: Duplicate identifier 'type'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,93):
00:54:16   TS2304: Cannot find name 'type'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,98):
00:54:16   TS1005: ',' expected.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,125):
00:54:16   TS2300: Duplicate identifier 'type'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,125):
00:54:16   TS2304: Cannot find name 'type'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,130):
00:54:16   TS1005: ',' expected.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,162):
00:54:16   TS2300: Duplicate identifier 'type'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,162):
00:54:16   TS2304: Cannot find name 'type'.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,167):
00:54:16   TS1005: ',' expected.

00:54:16   ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(399,192):
00:54:16   TS2300: Duplicate identifier 'type'.

@ghiscoding
Copy link
Contributor Author

@reduckted Thanks Dave! The following is my build log. I see that I need @types/trusted-types now too according to the above but what about the other errors

I think that you problem is something different and is potentially what I saw yesterday when I tried in a React app and that was caused by React itself. For example if you take this error from the console log

00:54:16 ERROR in C:/app/node_modules/dompurify/dist/purify.cjs.d.ts(245,9):
00:54:16 TS2304: Cannot find name 'TrustedHTML'.

if you then go to the source code and hover TrustedHTML and if you then press F12 (or "Go to Definition"), you will probably see that React replaced all the @types/trusted-types interfaces with empty interfaces and if you scroll on top of that file, you will see that they provide suggestion to add "lib": ["DOM"], to your tsconfig but that didn't work for me... if that is the problem you're having a React problem and that is not the same error as the original problem above that I'm having in Angular. If that is the case then I think you should open a separate issue (the source of the cause comes from the same, which is the addition of the types, but the problems are different hence why I think it should be a separate issue).

What I don't understand though in here, DOMPurify, is why @types/trusted-types isn't a dependency of DOMPurify? I think it should be and that would fix the React problem. This is how I used it in other projects of mine:

import type { TrustedHTML } from 'trusted-types/lib';

sanitize: (html: string) => string | TrustedHTML;

This would fix React since this would explicitly say TrustedHTML comes from @types/trusted-types. However it's not done this way in here, what we currently have is an expectation that the external user will install @types/trusted-types themselves and that TrustedHTML will somehow be available as a global type, it's not explicit enough I find. cc @reduckted

@ghiscoding
Copy link
Contributor Author

ghiscoding commented Nov 14, 2024

@reduckted so I tested a bit more and you're right at this point the patch that you have seems to be the only way to get ATTW working. However my Angular really doesn't like it, also I thought it might be because I'm using the legacy WebPack build but even with the new esbuild the error shows up. I also found that I'm falling on the CJS Types because my tsconfig is configured with "moduleResolution": "node" and I really don't want to use node16 because that brings a lot more problems. I also found that we can't do the rename that I mentioned above (d.ts to d.cts) because that only works when the project is configured with type: module... so I sadly don't know how to fix it either :(

I think the only way to make it all work would be to use "type": "module" but that would require a bit more work

EDIT

I got a working build with "type": "module" and the rename of d.ts to d.cts and Angular is happy... but ATTW fails with Incorrect default export because the project uses export default and the only way to fix this would be to use named exports (i.e.: import { sanitize } from 'dompurify' but that would be a breaking change. So... back to square one, even if on the long run I think default export should really be deprecated and named exports should be the way to go in the future. Rollup explains it well in their output exports config doc.

EDIT 2

@reduckted I found a very simple fix and push a PR #1022 and finally my build is working and everyone is happy :)

@cure53 cure53 closed this as completed in 2bc2188 Nov 14, 2024
cure53 added a commit that referenced this issue Nov 14, 2024
fix: ignore export assign cannot be used in module TS error, fixes #1018
@cure53
Copy link
Owner

cure53 commented Nov 14, 2024

Hey everyone :) The PRs have been merged, many thanks! Are we now good for another release?

@reduckted
Copy link
Contributor

Hey everyone :) The PRs have been merged, many thanks! Are we now good for another release?

I think so. At the very least, the changes that have been made should get it working for more people.

@asamuzaK
Copy link
Contributor

asamuzaK commented Nov 14, 2024

Re @types/trusted-types:
I think we should add @types/trusted-types to optionalDependencies (since it's only needed for typescript), or at least add a note in README that it is required now and user need to add it to devDependencies.
It's kinda breaking change.

Update:
Filed #1025

asamuzaK added a commit to asamuzaK/DOMPurify that referenced this issue Nov 14, 2024
asamuzaK added a commit to asamuzaK/DOMPurify that referenced this issue Nov 14, 2024
@asamuzaK
Copy link
Contributor

And one more.
Don't forget to update dependencies.
At least update "xo", v0.54.1 is vulnerable.

@ghiscoding
Copy link
Contributor Author

Hey everyone :) The PRs have been merged, many thanks! Are we now good for another release?

@cure53 Yes please 😃

@cure53
Copy link
Owner

cure53 commented Nov 20, 2024

3.2.1 is out, thanks everyone :) https://github.com/cure53/DOMPurify/releases/tag/3.2.1

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 a pull request may close this issue.

6 participants