HalitTalha / ng-material-extensions

Extended features for @angular/material components
Apache License 2.0
101 stars 52 forks source link

Reduce bundle size by async importing xslx #104

Closed stnor closed 3 years ago

stnor commented 3 years ago

Hi, We find this extension very useful. However it adds 1.4M to our bundle size... Which is A LOT.

It would be great if you could switch to lazy loading.

const xlsx = await import('xlsx');
or

import('xlsx').then(xlsx => {
    // do something with xlsx
});

There is also the "xlsx.mini.min.js" which is a fraction of the size. Won't that suffice?

See some discussion/hints here: https://github.com/SheetJS/sheetjs/issues/694

HalitTalha commented 3 years ago

Hi @stnor,

dynamic-import-proposal released with typescript 3.8 hence available with Angular 9.1 onwards. We will update our Angular version soon. We can consider shifting to dynamic imports where we can. The heavy bundle is actually caused by commonjs moduling of xlsx which prevents the bundler from keeping the bundle size optimized. Infact dynamic imports wouldn't be a fix for the final bundle size, it would be a nice improvement. Using the extra minified version would suffice. We can give it a try and include your suggestion in the next releaese.

Thanks for the suggestion.

stnor commented 3 years ago

Great! The longer you can defer the loading the better. In most use-cases I imagine that exporting tables are something the user does very infrequently. I'm on Angular 10 btw, but not using the cli, since I still have one ng1 app entry point left to migrate...

HalitTalha commented 3 years ago

The package is now depending on xlsx.mini.min. It reduced the bundle size a lot as expected. As for the dynamic imports, we eventually favor not adding the extra complexity it'll bring. It's better if the client code decides and lazy-loads mat-table-exporter functionality depending on the need.

stnor commented 3 years ago

Thanks! That's great!

I'd be happy to fix lazy loading in my app for MatTableExporter.

However, I don't really understand how the client should lazy-load mat-table-exporter functionality, given it is a directive, Can you share an example with a mat-table with excel export?

I'm using this now:

    @ViewChild(MatTableExporterDirective) exporter!: MatTableExporterDirective;

    export()
       this.exporter.exportTable('xlsx', {
            fileName: 'foo',
        });
    }
HalitTalha commented 3 years ago

It is actually to stay tuned with the xlsx api, I think of it like a regular interface rather than a javascript object. The reason to use the WritingOptions is originally the same motive behind using a strictly typed language . You are right, without it wrapping the xlsx calls for lazy handling wouldn't be a big deal because only the exporter.service layer is coupled with xlsx however copy pasting the interface definition wouldn't be a fair tradeoff for my point of view. Yet I still understand that it could seem a no-brainer for those who have to optimize their bundle size at the best and the people with JS background. :) Maybe we can find out another way later on.

HalitTalha commented 3 years ago

Btw, the package with minifed version is released as the latest 10.2.0

stnor commented 3 years ago

That's a shame regarding the lazy loading stance. Even though we just shrunk your bundle size with a factor 10x, it is still unacceptably large for the value it brings given the frequency of table exports in my opinion.

I'm not sure it's a good design practice to to use the api of an underlying dependency exposed as your own. If the xlsx dep wasn't monolitic and I would somewhat understand, but given that it is, I'd strongly consider copying the file.

I'd rather not have to fork a good and actively maintained project, but hey that's life.

HalitTalha commented 3 years ago

I'm not sure it's a good design practice to to use the api of an underlying dependency exposed as your own. If the xlsx dep wasn't monolitic and I would somewhat understand, but given that it is, I'd strongly consider copying the file.

The exposed type is an extension of xlsx WritingOptions, as you pointed this is an indication of strong coupling between the projects however this is a design decision that has pros and cons and it gives the opportunity of leveraging all the options that xlsx provides through WritingOptions.

I'm sorry you had to fork.

stnor commented 3 years ago

No worries. Thanks for an otherwise great lib.

stnor commented 3 years ago

It was quite easy. I just made the WorksheetExporter.createContent and workSheetToContent async and added a few awaits starting from CdkTableExporter.exportExtractedData

The imports like WorkSheet and WritingOptions could be retained without loading xlsx upfront.

HalitTalha commented 3 years ago

Sounds great. If it's OK, we appreciate any contribution :)

stnor commented 3 years ago

If you like I can make a pull request later. As of present I only copied the files into my project and fixed it there.

HalitTalha commented 3 years ago

It was quite easy. I just made the WorksheetExporter.createContent and workSheetToContent async and added a few awaits starting from CdkTableExporter.exportExtractedData

The imports like WorkSheet and WritingOptions could be retained without loading xlsx upfront.

Followed the same approach. As you also pointed out, type imports didn't end up in the bundle since they are elided. I'll make a new release with lazy loading support. Switching right into xlsx.mini.min on the other hand turned out to be a bad idea since it is a lightweight version. I'm planning to make it optional through a module configuration parameter. Depending on the parameter, mini.min version will be imported dynamically. How does this sound?

stnor commented 3 years ago

Cool. Sorry, totally forgot that PR. I’d be willing to test when done.

Let me know!

On Sun, 25 Apr 2021 at 23:16, Halit Talha TÜRE @.***> wrote:

Reopened #104 https://github.com/HalitTalha/ng-material-extensions/issues/104.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HalitTalha/ng-material-extensions/issues/104#event-4641651342, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEJAD7MIZNZR6N4UM5GUK3TKSBDZANCNFSM42UXR6JQ .

HalitTalha commented 3 years ago

Cool. Sorry, totally forgot that PR. I’d be willing to test when done. Let me know! On Sun, 25 Apr 2021 at 23:16, Halit Talha TÜRE @.***> wrote: Reopened #104 <#104>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#104 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEJAD7MIZNZR6N4UM5GUK3TKSBDZANCNFSM42UXR6JQ .

Thank you for the help :)

Btw, with the latest version 10.2.3, you can switch to xlsx.mini.min with something like this:

@NgModule({
  declarations: [
    AppComponent
  ],
  imports: [
    MatTableExporterModule.forRoot({xlsxLightWeight: true}),
  ],
  providers: [],
  bootstrap: [AppComponent]
})
HalitTalha commented 3 years ago

One caveat however, since xlsx.mini.min and xlsxare being imported dynamically depending on the user preference, webpack happens to bundle them both. I spent a lot of time to figure a way out but couldn't find any. I'm always open to new ideas.

resim

stnor commented 3 years ago

Aw, that's a shame. I don't think the lazy loading will help if that isn't resolved, as it will both be part of the vendor bundle and loaded again as lazy.

I've had similar issues when working on lazy loading, but I got it to work when I copied the stuff I needed from your project. I always use BundleAnalyzerPlugin to verify, but it doesn't tell you "why" though.

On Sun, Apr 25, 2021 at 11:53 PM Halit Talha TÜRE @.***> wrote:

One caveat however, since xlsx.mini.min and xlsx are being imported dynamically depending on the user preference, webpack happens to bundle them both. I spent a lot of time to figure a way out but couldn't find any. I'm always open to new ideas.

[image: resim] https://user-images.githubusercontent.com/3686045/116010711-5d0dfd80-a629-11eb-918a-b042e23609eb.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HalitTalha/ng-material-extensions/issues/104#issuecomment-826395839, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEJADYF45ETKCZ25DEVGT3TKSFMHANCNFSM42UXR6JQ .

stnor commented 3 years ago

I'm attaching my changes here, perhaps that can help. exporter.zip

HalitTalha commented 3 years ago

Oh, sorry for not being clear enough. They are not part of the vendor bundle, they are just lazy chunks. What I was trying to tell was, they both are served as lazy chunks, since webpack couldn't guess which one will be required at run time.

So this is not the case ;

as it will both be part of the vendor bundle and loaded again as lazy.

stnor commented 3 years ago

Why is vendor 4MB then?

HalitTalha commented 3 years ago

That wasn't a prod build actually, I had just shared to show the lazy and vendor chunks. If you spot something otherwise, please let us know anyway.

With prod build:

resim

And this is the build-stats:

resim

stnor commented 3 years ago

Ok, great! Looks good to me!

On Mon, Apr 26, 2021 at 2:01 PM Halit Talha TÜRE @.***> wrote:

That wasn't a prod build actually, I had just shared to show the lazy and vendor chunks.

With prod build: [image: resim] https://user-images.githubusercontent.com/3686045/116078759-abf67a00-a69f-11eb-8b4a-609635538c24.png

And this is the build-stats: [image: resim] https://user-images.githubusercontent.com/3686045/116079011-00015e80-a6a0-11eb-9860-31f3bb5a49fd.png

If you spot something otherwise, please let us know anyway.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HalitTalha/ng-material-extensions/issues/104#issuecomment-826778755, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEJADZETWKR63MCO34CV6LTKVI2HANCNFSM42UXR6JQ .