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

Reduce bundle size by async importing xslx #104

Closed
stnor opened this issue Apr 9, 2021 · 22 comments
Closed

Reduce bundle size by async importing xslx #104

stnor opened this issue Apr 9, 2021 · 22 comments
Labels
enhancement feature request New feature or request

Comments

@stnor
Copy link

stnor commented Apr 9, 2021

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: SheetJS/sheetjs#694

@HalitTalha
Copy link
Owner

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
Copy link
Author

stnor commented Apr 9, 2021

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 HalitTalha added enhancement feature request New feature or request labels Apr 9, 2021
HalitTalha added a commit that referenced this issue Apr 11, 2021
@HalitTalha
Copy link
Owner

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
Copy link
Author

stnor commented Apr 11, 2021

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
Copy link
Owner

HalitTalha commented Apr 11, 2021

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
Copy link
Owner

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

@stnor
Copy link
Author

stnor commented Apr 11, 2021

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
Copy link
Owner

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
Copy link
Author

stnor commented Apr 11, 2021

No worries. Thanks for an otherwise great lib.

@stnor
Copy link
Author

stnor commented Apr 11, 2021

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
Copy link
Owner

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

@stnor
Copy link
Author

stnor commented Apr 11, 2021

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
Copy link
Owner

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?

@HalitTalha HalitTalha reopened this Apr 25, 2021
@stnor
Copy link
Author

stnor commented Apr 25, 2021 via email

@HalitTalha
Copy link
Owner

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
Copy link
Owner

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.

resim

@stnor
Copy link
Author

stnor commented Apr 25, 2021 via email

@stnor
Copy link
Author

stnor commented Apr 25, 2021

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

@HalitTalha
Copy link
Owner

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
Copy link
Author

stnor commented Apr 26, 2021

Why is vendor 4MB then?

@HalitTalha
Copy link
Owner

HalitTalha commented Apr 26, 2021

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
Copy link
Author

stnor commented Apr 26, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feature request New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants