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

Split up compiling and parsing code and bundle minimal production bundle #548

Closed
wants to merge 5 commits into from

Conversation

mortenson
Copy link
Contributor

Addresses #530, and is branched on top of #542

Currently, the code in Twig.js that's related to compiling templates and parsing compiled templates exist in the same source files, and the distributed files include the ability to both compile and parse templates.

As a result, the twig.min.js file that clients (browsers) use is ~86K, which is quite large for a production site.

Twig.js also includes the ability to pre-compile templates, which is used by projects like twig-loader to inline templates in Javascript when they're required with statements like require('my-template.twig'). This results in a performance improvement as no client will ever have to compile templates, which leads to faster rendering.

If all templates on the client are pre-compiled, the client does not need any Twig.js compilation code, so it makes sense that Twig.js would provide an alternative distributed file that only includes the minimal code needed to parse compiled templates.

This PR refactors the src/ file structure so that compilation and parsing code is decoupled, and adds a new entrypoint which only includes code related to parsing. The entrypoint maps to a new distributed file, twig.production.min.js, which is 30K smaller than twig.min.js!

This is mostly a code-move, but there is a code change here in that logic and expression definitions can now be partially defined or overridden. This means that if you just wanted to define (or re-define) the compile or parse methods (or any property besides ID) for a definition, you could do that at runtime or during the build process. This is how splitting up *definitions.compile.js and *definitions.parse.js files works in the PR.

I think the main negative here is that because code is moved around, the open PRs will need to be re-merged if they touched refactored methods. There are only 9 PRs open, and most of them need work or have conflicts already.

@willrowe
Copy link
Collaborator

  • Thanks for all your work on this @mortenson!
  • I like the idea put forward with this. The only thing that stood out to me while looking over it was in regards to the logic token definitions. It would be great if they did not have to be split.
    • Do you know what the difference in size would be between splitting and not splitting?
    • Is it possible to configure Webpack to ignore the compile related items on the definitions or define them in such a way to accomplish the same thing? Maybe have them in a separate module and split them on the export?
  • A version 2.0 will probably be necessitated by some other PRs, so I think it would make sense to incorporate this into that release.

@mortenson
Copy link
Contributor Author

Do you know what the difference in size would be between splitting and not splitting?

twig.production.js grows by about 15k if you always bundle the compile and parse methods.

Is it possible to configure Webpack to ignore the compile related items on the definitions or define them in such a way to accomplish the same thing? Maybe have them in a separate module and split them on the export?

I started to look into this and I think so, but am not really sure how to accomplish it. You can exclude entire modules from bundling, but that's usually used for marking libraries as external. I think with a plugin you may be able to modify the bundle before it's saved to a file but I don't know how to accomplish that. If it's possible that would be a much easier path forward than this refactor.

@willrowe
Copy link
Collaborator

willrowe commented May 11, 2018

twig.production.js grows by about 15k if you always bundle the compile and parse methods.

Ok, that's not great, so we would definitely want to exclude them.

I think with a plugin you may be able to modify the bundle before it's saved to a file but I don't know how to accomplish that.

I was thinking the same thing.

If it's possible that would be a much easier path forward than this refactor.

Good point, if we were to create plugin to split the logic token definitions we might as well do it for the rest. I started looking into how difficult this would be, I'll report back with my findings.

@PolyPik
Copy link
Contributor

PolyPik commented May 21, 2019

@mortenson I really wished that this PR was merged sooner, because a lot of changes have occurred since May of last year.

In Issue #622, I proposed that the global object be broken up into modules that will be assembled into a single bundle by Webpack. Should this PR be merged prior to such a massive refactor so that the work is not wasted or would it be easier to just redo the work after the refactor has occurred?

@willrowe
Copy link
Collaborator

Separating out sections of the project should be easier after they are broken up into modules.

Also, I'm currently working on completely rewriting how templates are compiled and the tokens are generated in order to address some compatibility issues with the PHP version. I have been keeping in mind modularity while doing so, which should make it easier to split as well.

Side note: I hit some issues while writing tests for the new token compiler which would have been non-issues if modules were used, so I'm definitely looking forward to that.

@RobLoach
Copy link
Collaborator

I'd like to revisit this. Having the compiled bundle used by the npm package had proven problematic. We can package the bundled files, but should likely have the main point to the source twig.js.

@willrowe
Copy link
Collaborator

Closing this for now due to the complexity of actually making these changes at this point. This can be revisited once the code has been cleaned up a bit and modernized. For now it can be tracked in #530.

@willrowe willrowe closed this Jun 27, 2022
@larowlan larowlan mentioned this pull request Jan 31, 2023
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 this pull request may close these issues.

4 participants