plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
246 stars 187 forks source link

PLIP: Meta bundles generation #1277

Closed ebrehault closed 8 years ago

ebrehault commented 8 years ago

Proposer : Eric Bréhault

Seconder :

Abstract

In Plone 5, resources bundles are loaded separately in all the portal pages. This PLIP proposes to be able to merge them in meta bundles.

Motivation

When a Plone site uses 10 add-ons, all the pages will contain 10 extras SCRIPT tags and 10 extra LINK tags (plus the default ones provided by Plone). This is very bad for performances, we need to be able to aggregate them.

Assumptions

Proposal & Implementation

Disclaimer

Plone integrators and Plone add-on developers need a simple story:

Consequently, we do not want to interact with the existing bundling mechanism as it requires all the front-end dev tooling.

The objective here is just to concatenate existing JS or CSS bundles together, which should be a basic string processing operation we can trigger easily from a GenericSetup step.

Proposal

Products.CMFPlone will define 2 meta bundles:

Each meta bundle will produces 2 outputs (one JS and one CSS).

Note: I do not think we need to be able to declare other meta bundles. If we need to provide some specific resources in a given page or in a given condition, regular bundles should be satisfying, using the expression property, or using add_bundle_on_request http://docs.plone.org/adapt-and-extend/theming/resourceregistry.html#adding-or-removing-bundles-from-a-request .

Bundles will have a new property named merge_with which possible values will be default, logged-in or nothing:

The meta bundle generation is done by a GenericSetup step. As bundles can be defined or modified TTW, we will also provide a "Merge bundles for production" button in the Resource registry that will re-generate the meta bundles.

Implementation

Meta bundles are stored in the "persistent" IResourceDirectory, just like as the current bundle cooking does at the moment. They are produced by aggregating together all the bundle contents in a single string.

Deliverables

N/A

Participants

Eric Bréhault

jensens commented 8 years ago

:+1: That is indeed the missing piece needed to reduce headache with js/css resources and addons.

Just two things:

1) the property name `merged-in - I'am sure we could find a better one.... - at least to me it sounds strange.

2) Cite:

The meta bundle generation is done by a GenericSetup step if a file named merge_bundles.txt is present in the imported profile.

I am not sure if this file is needed at all. Its a bit redundant: Why don't we look at registry.xml if a 'merged-in' property is used? It's a bit more difficult, but should be possible.

ebrehault commented 8 years ago

@jensens, I chose merged-in as I do not want to expose the meta bundle concept in anyway, because it sounds scary. The proper term would be bundle of course (which would be similar to bundles we had in Plone 4's portal_javascript), but this term is already used for a different thing in Plone 5. Would you prefer aggregate or something like that?

Regarding the use of merge_bundles.txt to trigger the GS step: I agree we could look into registry.xml, but I was not sure it is ok to use the same profile file for 2 different steps (the regular registry step + the new metabundle step), what do you think?

mauritsvanrees commented 8 years ago

Name ideas: merge-in, merge-to, combine, combined-bundle, master-bundle, full-bundle, main-bundle. join.

For clarity: If I am logged in I get both the default and the logged-in bundle, right?

Having two packages that read a single GS file, would be a first I think. There is no technical reason that this would not work, but it could be confusing.

Do we need any file at all? Can't the metabundle step simply generate the new bundles always, compare them with what is currently stored in the registry and only save them when there is a difference? This would avoid the need to tell add-on authors to add yet another file.

If checking the config and concatenating the bundles and comparing them takes say five seconds, then this does increase the time needed for activating an add-on. If that is the case, we could indeed check for existence of registry.xml.

For speed, the step might want to check if a Plone Site is being created, and skip execution then: somewhere in the plone-final step we could call the needed code explicitly once. But we can revisit this if we see that install time is a problem.

ebrehault commented 8 years ago

For clarity: If I am logged in I get both the default and the logged-in bundle, right?

Yes

Do we need any file at all?

Good point. Actually, I think we don't.

jensens commented 8 years ago

I like the terms combined-bundle or main-bundle - or maybe bundle-aggregate or bundle-collection. Reason: We stick to the term bundle, because finally thats what it is, and then refine what kind of bundle it is.

mauritsvanrees commented 8 years ago

I vote jbob: Just a Bunch Of Bundles. ;-)

Nah.

jensens commented 8 years ago

:-D

thet commented 8 years ago

What about this:

mauritsvanrees commented 8 years ago

merge_with: fine with me.

On Plone 4, in production mode the bundles were preserved between restarts. I guess if no compiled/merged bundle yet existed, one would get created when the first page was requested.

When Zope restarts, I would not mind if the bundles get recalculated, as new versions of packages may have been added. But I think the consensus, or at least the current behavior, is to not regenerate them, as it may really just have been a restart, without any changes. A needless regeneration would add seconds to the restart time, so possibly a few extra seconds of down time, which we want to avoid.

Concatenating once after a registry.xml change is roughly what I am suggesting too, although I would simply put it in an import step.

Note that if you write an upgrade step in your add-on to apply the plone.app.registry import step, you will not get any reconcatenation: you should then apply the new metabundle step too.

ebrehault commented 8 years ago

Ok let's go with combined-bundle, I update the description.

ebrehault commented 8 years ago

Sorry I missed the last messages in the discussion before I answered: so yes ok merge_with is good to me too. But regarding concatenation time, I really prefer to stick with the GS step import rather than Zope restart, just because it makes more sense to manage it with GS considering it depends uniquely on confirguration.

davilima6 commented 8 years ago

+1 for concatenation after GS step changes values for resource registry related keys. Zope startup must be fast (zcml/zca is probably biggest culprit but we should aim for quick automatic restarts/reloads, as Django's).

MrTango commented 8 years ago

Doing this on Zope startup will propably not work, because the process of merging/compiling needs a browser. I was asking @bloodbare to automaticly rebuild a bundle if someone uses the registry to register a resource into an existing bundle like the plone-bundle. Because this must work on install time of an add-on without special user action and think. But he said it is not so easy to solve because of the needed browser.

If we want to do this without a browser we need probably a new dependency in Plone like the JS stack of npm/gulp whatever. And this is proably not what we want.

ebrehault commented 8 years ago

@MrTango

Doing this on Zope startup will propably not work, because the process of merging/compiling needs a browser.

Actually it does not need a browser, because here we are not building bundles but just aggregating existing bundles together. That's what I explained in the disclaimer: we do not want to involve the bundling mechanism here, just use existing bundles.

frisi commented 8 years ago

at the mockup (aka resource registry) training in bucharest i - and i think it's safe to say that for most of the other attendees too - had a hard time figuring out how the resource registry is supposed to work.

we proposed something similar as this plip does (being able to combine bundles easily) in a different way that can't be realized (easily, or at all) https://github.com/plone/Products.CMFPlone/issues/1163

for what it's worth - i tried to summarize the steps currently necessary for an integrator to combine the resources of different bundles into a new mysite-plone and mysite-plone-logged-in bundle in the section Optimized Bundles in https://github.com/plone/documentation/pull/482

@ebrehault: if i understand this proposal correctly we're still including jquery (and possibly other resources required by and therefore integrated in multiple bundles) multiple times. maybe we can get around this by excluding jquery and other stub_js_modules when building bundles ttw as it's done when using the console script (see https://github.com/plone/Products.CMFPlone/pull/1210)

ebrehault commented 8 years ago

@frisi no we are not including jquery (or other resources) multiple time. @vangheem made a change recently, we now have a new attribute named depends which makes sure we do not include resources in a given bundle if they are already provided by its dependency. Like here for instance: https://github.com/plone/Products.CMFPlone/blob/master/Products/CMFPlone/profiles/dependencies/registry.xml#L1079 we tell the bundling mechanism to not put in the plone-logged-in bundle resources already part of plone bundle.

Consequently, if the depend attribute is properly used, the meta-bundles defined in his PLIP should not contain resources multiple time.

ebrehault commented 8 years ago

@frisi, sorry I told you wrong: what @vangheem changed is the stub_js_modules attribute (not the depend attribute), see https://github.com/plone/Products.CMFPlone/pull/1210, so that's the one we must use to avoid multiple inclusion of same js, and right now in the plone core bundles, it is not the case, so we do include jQuery several time.

ebrehault commented 8 years ago

Documentation: https://github.com/plone/documentation/tree/plip-1277-meta-bundles-generation

bloodbare commented 8 years ago

Great work !

yurj commented 8 years ago

Please, don't add the default and logged-in. Usually logged-in are few things we can load anyway.

ebrehault commented 8 years ago

@yurj I am not sure I understand what you mean: do you mean we do not need the logged-in meta bundle, the default will be just enough ? That might be true for a vanilla Plone, but add-ons might need to add extra resources just for logged-in users.

thet commented 8 years ago

default and logged in makes perfect sense: plone-compiled.js is 644K, minified 231K plone-logged-in-compiled.js is 2.2M, minified 936K

But one can reconfigure all this anyways and offer only one bundle for anonymous and logged in users.

thet commented 8 years ago

@plone/framework-team please vote for the proposal of this PLIP: https://docs.google.com/spreadsheets/d/15Cut73TS5l_x8djkxNre5k8fd7haGC5OOSGigtL2drQ/edit?pli=1#gid=3 If positive, this PLIP can directly got to the "under review" row, be reviewed, voted again and merged.