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

Angular esbuild error on DomPurify with rowDetailView and no pre/post template #1362

Closed
5 tasks done
jr01 opened this issue Feb 6, 2024 · 5 comments · Fixed by #1363
Closed
5 tasks done

Angular esbuild error on DomPurify with rowDetailView and no pre/post template #1362

jr01 opened this issue Feb 6, 2024 · 5 comments · Fixed by #1363

Comments

@jr01
Copy link
Collaborator

jr01 commented Feb 6, 2024

Describe the bug

Hi!

Angular esbuild with rowDetailView gives a runtime error:

global-error.handler.ts:37 TypeError: (void 0) is not a function
    at addonOptions.preTemplate (angular-slickgrid.mjs:313:57)
    at SlickRowDetailView2.expandDetailView (slickRowDetailView.js:231:75)
    at SlickRowDetailView2.handleAccordionShowHide (slickRowDetailView.js:564:22)
    at SlickRowDetailView2.toggleRowSelection (slickRowDetailView.js:647:18)
    at SlickRowDetailView2.handleClick (slickRowDetailView.js:587:22)
    at SlickEvent.notify (slickCore.js:155:51)
    at SlickGrid.trigger (slickGrid.js:2248:20)
    at SlickGrid.handleClick (slickGrid.js:4148:20)
    at _ZoneDelegate.invokeTask (zone.js:402:31)
    at core.mjs:14452:55

With esbuild namespace imports are not supported: https://angular.io/guide/esbuild#esm-default-imports-vs-namespace-imports

import * as DOMPurify from 'dompurify'; should be changed to import DOMPurify from 'dompurify';

Reproduction

  1. Download Angular-Slickgrid and install packages
git clone https://github.com/ghiscoding/Angular-Slickgrid
cd Angular-Slickgrid
yarn install
  1. Change "builder": "@angular-devkit/build-angular:browser" to "builder": "@angular-devkit/build-angular:browser-esbuild" in angular.json

  2. yarn start ( notice the lightning fast build time 😄 )

  3. Browse to: http://localhost:4300/#/rowdetail

  4. Expand the first row

  5. Notice expansion doesn't work and console log shows the error.

  6. Change to import DOMPurify from 'dompurify'; in slickRowDetailView.ts and save the file.

  7. Wait a second for rebuild, expand first row, notice it works :)

We use this workaround for now:

	onAngularGridCreated(angularGrid: AngularGridInstance) {
		const slickRowDetailView = angularGrid.extensionService.getExtensionInstanceByName(ExtensionName.rowDetailView);
		const slickRowDetailViewOptions = slickRowDetailView.getOptions();
		slickRowDetailViewOptions.preTemplate = () => '<div class="container_loading"></div>';
		const datasetIdPropName = this._gridOptions.datasetIdPropertyName ?? 'id';
		slickRowDetailViewOptions.postTemplate = (itemDetail: any) => `<div class="container_${itemDetail[datasetIdPropName]}"></div>`;

I can submit a PR to fix this one liner if you want?

Expectation

No response

Environment Info

Angular 17.1
Angular-Slickgrid 7.2.0

Validations

@ghiscoding
Copy link
Owner

ghiscoding commented Feb 6, 2024

DOMPurify doesn't work with import DOMPurify from 'dompurify'; with regular Angular bundler (not esbuild), it works fine in Slickgrid-Universal but not in here, it really has to be import * as DOMPurify. The DOMPurify project tried to change their exports so that it works better but it actually caused a regression mostly for Angular users like my project causing this Angular-Slickgrid issue #1348 and tracked in DOMPurify regression Build error in angular app.

So I'm afraid that there is no fix, I did try to use import DOMPurify from 'dompurify' but that didn't work in Angular (maybe it does when using esbuild but it fails with regular Angular bundler which is what most user still use). DOMPurify never had a full ESM approach

You could maybe try this old Rollup patch that I used in the past for other dependency like MomentJS, this might work (you could submit a PR if it does)

// patch to fix rollup "moment has no default export" issue,
// documented at:  https://github.com/rollup/rollup/issues/670
import * as dompurify_ from dompurify';
const DOMPurify = (dompurify_ as any)['default'] || dompurify_; 

in summary, it's a DOMPurify problem that existed forever (they're still using a CommonJS approach and even if they tried, they still haven't found the best approach)

EDIT

I was actually using that patch in Slickgrid-Universal 3.x but stop using it in 4.x. I might have to put it back in place though at least in here, see this line for 3.x

@jr01
Copy link
Collaborator Author

jr01 commented Feb 7, 2024

Ah, sorry, I totally didn't think about actually running yarn build in Angular-Slickgrid and testing if the package works...

Doing that gives:

------------------------------------------------------------------------------
Building entry point 'angular-slickgrid'
------------------------------------------------------------------------------
✖ Compiling with Angular sources in Ivy partial compilation mode.
src/app/modules/angular-slickgrid/extensions/slickRowDetailView.ts:7:8 - error TS1259: Module '"C:/repos/GitHub/Angular-Slickgrid/node_modules/@types/dompurify/index"' can only be default-imported using the 'allowSyntheticDefaultImports' flag

7 import DOMPurify from 'dompurify';
         ~~~~~~~~~

  node_modules/@types/dompurify/index.d.ts:4:1
    4 export = DOMPurify;
      ~~~~~~~~~~~~~~~~~~~
    This module is declared with 'export =', and can only be used with a default import when using the 'allowSyntheticDefaultImports' flag.  

Which is odd, since that flag seems to be set.

I'm not sure about your workaround though, I'm a bit hesitant to introduce a hack.

Taking a step back and starting fresh from https://angular.io/guide/creating-libraries

ng new my-workspace --no-create-application
cd my-workspace
ng generate library my-lib

cd projects/my-lib
npm i dompurify
npm i --save-dev @types/dompurify

Add to my-lib.component.ts

import DOMPurify from 'dompurify';
...
 ngOninit() {
    const clean = DOMPurify.sanitize('<script>alert("Hello World!")</script>');
    console.log(clean);
  }

So I cd ../..; ng build:

------------------------------------------------------------------------------
Building entry point 'my-lib'
------------------------------------------------------------------------------
✖ Compiling with Angular sources in Ivy partial compilation mode.
projects/my-lib/src/lib/my-lib.component.ts:2:8 - error TS1259: Module '"C:/repos/experiments/angular/lib/my-workspace/projects/my-lib/node_modules/@types/dompurify/index"' can only be default-imported using the 'allowSyntheticDefaultImports' flag

2 import DOMPurify from 'dompurify';
         ~~~~~~~~~

  projects/my-lib/node_modules/@types/dompurify/index.d.ts:4:1
    4 export = DOMPurify;
      ~~~~~~~~~~~~~~~~~~~
    This module is declared with 'export =', and can only be used with a default import when using the 'allowSyntheticDefaultImports' flag.

So I add "allowSyntheticDefaultImports": true to tsconfig.lib.json and run ng build:

------------------------------------------------------------------------------
Building entry point 'my-lib'
------------------------------------------------------------------------------
✔ Compiling with Angular sources in Ivy partial compilation mode.
✔ Generating FESM bundles
✔ Copying assets
⚠ Distributing npm packages with 'dependencies' is not recommended. Please consider adding dompurify to 'peerDependencies' or remove it from 'dependencies'.
✖ Writing package manifest
Dependency dompurify must be explicitly allowed using the "allowedNonPeerDependencies" option.

So I add "allowedNonPeerDependencies": [ "dompurify" ] tong-package.json`:

------------------------------------------------------------------------------
Building entry point 'my-lib'
------------------------------------------------------------------------------
✔ Compiling with Angular sources in Ivy partial compilation mode.
✔ Writing FESM bundles
✔ Copying assets
ℹ Removing devDependencies section in package.json.
✔ Writing package manifest
✔ Built my-lib

------------------------------------------------------------------------------
Built Angular Package
 - from: C:\repos\experiments\angular\lib\my-workspace\projects\my-lib
 - to:   C:\repos\experiments\angular\lib\my-workspace\dist\my-lib
------------------------------------------------------------------------------

Now I just need to figure out why in Angular-Slickgrid the ng-packagr is not picking up the allowSyntheticDefaultImports...

@jr01
Copy link
Collaborator Author

jr01 commented Feb 7, 2024

One step further.

Adding tsconfig.lib.json with

/* To learn more about this file see: https://angular.io/config/tsconfig. */
{
  "extends": "./tsconfig.json",
  "compilerOptions": {
    "outDir": "./out-tsc/lib",
    "allowSyntheticDefaultImports": true,
    "declaration": true,
    "declarationMap": true,
    "inlineSources": true,
    "types": []
  },
  "exclude": [
    "**/*.spec.ts"
  ]
}

and adding tsconfig.lib.prod.json with:

/* To learn more about this file see: https://angular.io/config/tsconfig. */
{
  "extends": "./tsconfig.lib.json",
  "compilerOptions": {
    "declarationMap": false
  },
  "angularCompilerOptions": {
    "compilationMode": "partial"
  }
}

to Angular-Slickgrid and changing in package.json to "build": "ng-packagr -p ng-package.json -c tsconfig.lib.prod.json", and running yarn build gives:

$ ng-packagr -p ng-package.json -c tsconfig.lib.prod.json
Building Angular Package

------------------------------------------------------------------------------
Building entry point 'angular-slickgrid'
------------------------------------------------------------------------------
✔ Compiling with Angular sources in Ivy partial compilation mode.
✔ Generating FESM bundles
✔ Copying assets
ℹ Removing scripts section in package.json as it's considered a potential security vulnerability.
ℹ Removing devDependencies section in package.json.
✔ Writing package manifest
✔ Built angular-slickgrid

------------------------------------------------------------------------------
Built Angular Package
 - from: C:\repos\GitHub\Angular-Slickgrid
 - to:   C:\repos\GitHub\Angular-Slickgrid\dist
------------------------------------------------------------------------------

Build at: 2024-02-07T07:57:11.351Z - Time: 9162ms

$ npm-run-all copy:i18n
$ copyfiles -f src/assets/i18n/*.json dist/i18n
Done in 25.61s.

I'm not yet sure about all the details here (settings in the tsconfig.lib.json files) ...and also need to test out the produced package.

@jr01
Copy link
Collaborator Author

jr01 commented Feb 7, 2024

Just ng-packagr -p ng-package.json -c tsconfig.json also solves the problem.

ng-packagr by default uses an internal tsconfig, see https://github.com/ng-packagr/ng-packagr/blob/main/docs/override-tsconfig.md and since Angular-Slickgrid is not using ng build the tsconfig.json (with the allowSyntheticDefaultImports: true) needs to be explicitly provided.

Also I tested the produced package in our app, all looks good :)

PR incoming...

jr01 added a commit that referenced this issue Feb 7, 2024
…sbuild error on DomPurify with rowDetailView and no pre/post template #1362
@ghiscoding
Copy link
Owner

ghiscoding commented Feb 7, 2024

😮 wow that is a lot of investigation and knowing that ng-packgr use their tsconfig explains why it worked in all my other projects SlickGrid project except in Angular. I was totally unaware of this side effect by ng-packagr. Your work here is really awesome in finding a workable fix 👏🏻

@ghiscoding ghiscoding changed the title Angular esbuild error on DomPurify with rowDetailView and no pre/post template fix: Angular esbuild error on DomPurify with rowDetailView and no pre/post template Feb 13, 2024
@ghiscoding ghiscoding changed the title fix: Angular esbuild error on DomPurify with rowDetailView and no pre/post template Angular esbuild error on DomPurify with rowDetailView and no pre/post template Feb 13, 2024
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.

2 participants