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 #464

Closed
MickL opened this issue Dec 6, 2019 · 12 comments
Closed

Reduce bundle size #464

MickL opened this issue Dec 6, 2019 · 12 comments

Comments

@MickL
Copy link

MickL commented Dec 6, 2019

Would it be possible to reduce the total file size of this module? When I compile with webpack it results in 1.1mb minified.

I know bundle-file-size is normally not a requirement in Node.js, but for example an serverless environment like Cloudflare Worker do have a limit of 1mb.

Test-code I compiled:

const {TwingEnvironment, TwingLoaderArray} = require('twing');

let loader = new TwingLoaderArray({
    'index.twig': 'Hello {{ name }}!'
});
let twing = new TwingEnvironment(loader);

twing.render('index.twig', {name: 'Fabien'}).then((output) => {
    // do something with the output
});
@ericmorand
Copy link
Member

The main culprit here is the encoding conversion support that is part of Twig specification (see the encode filter for example).

But it should be possible to have the encoder module removed from the bundle if you don't need encoding support in your application.

Twing is using iconv-lite to support encoding.

@MickL
Copy link
Author

MickL commented Dec 6, 2019

I dont understand where this encode is used exactly and what feature would be missing then.

Could you create an module we can import that does not use encoding?

Btw. I didnt understand why I need TwingLoaderArray and TwingEnvironment. I would have liked to just use twing.render(). If thats possible that would save bundle size for me, too.

@ericmorand
Copy link
Member

The encoding support is needed for the convert_encoding filter:

https://twig.symfony.com/doc/3.x/filters/convert_encoding.html

You should be able to use Webpack to replace iconv_lite module by a fake module that returns the input as output to dramatically decrease the bundle size.

This is not Twing responsibility to provide custom set of specifications. Twing implements the specification strictly,.without compromise. Bundlers provide ways to remove module and are responsible of removing the features you don't need.

@ericmorand
Copy link
Member

As an alternative, grab the sources, modify src/lib/extension/core/filters/convert_encoding.ts to remove the importation of iconv_lite and just return string in the function.

Than install the dependencies and run npm run bundle and you'll get a nice bundle without encoding conversion support.

@kctang
Copy link

kctang commented Dec 13, 2019

Any chance this can be done as an optional dependency where twing detects if iconv_lite exists?

Making this optional will be useful for scenarios where twing is used in browser, where downloaded payload size matters.

Thanks for this nice package and hope you consider this. ;-)

@ericmorand
Copy link
Member

Well iconv_lite exists since it is bundled with the rest of the app.

Maybe someone can maintain a lite version of Twing without encoding conversion support?

I keep on thinking that having iconv_lite replaced by a stub by your module bundler is the best way to go. I can provide help with that. Even write a Wiki about it.

@dorian-marchal
Copy link

I can provide help with that. Even write a Wiki about it.

Thanks, it would be nice to have an "official" and supported way of stripping iconv_lite from bundles.
I'm not familiar with webpack (and bundlers in general), but it sounds like something that could break easily due to a Twing update.
Is this even possible to support something like that officially in a "bundler agnostic" way?

@ericmorand
Copy link
Member

I'm not familiar with webpack (and bundlers in general), but it sounds like something that could break easily due to a Twing update.

It could if for example we change the encoding library to something else. But not easily because it's not gonna change anytime soon.

But what I can do is provide two bundles with the package. One with encoding conversion support and one without. And update the doc accordingly. That seems reasonable to me.

@MickL
Copy link
Author

MickL commented Dec 16, 2019

From my perspective it would be great to have a different import which inits twing without inconv_lite instead of hacking it away when bundling.

@ericmorand
Copy link
Member

My testing shows than removing iconv-lite decreases the bundle from 1.3MB to 1.0MB with Browserify. Not bad. I guess results would be even better with Webpack.

@MickL
Copy link
Author

MickL commented Dec 20, 2019

Still this is (too) huge :/ Is it possible to refactor twing so each developer can import only the things we actually need? Keyword: tree shaking

@MickL
Copy link
Author

MickL commented Jan 17, 2020

@ericmorand

My testing shows than removing iconv-lite decreases the bundle from 1.3MB to 1.0MB with Browserify.

Currently it is more than 4MB which is an rediculous amount of code and Webpack throws a warning:

WARNING in ./node_modules/twing/dist/cjs/lib/cache/filesystem.js 35:28-47
Critical dependency: the request of a dependency is an expression
 @ ./node_modules/twing/dist/cjs/main.js
 @ ./src/services/twig-template-engine.service.ts
 @ ./src/services/response.service.ts
 @ ./src/main.ts

Could you provide this an TwingEnvironment export that does not contain iconv_lite?
Are there more options to reduze the bundle size?

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

4 participants