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

PLIP: Meta bundles generation #1277

Closed
ebrehault opened this issue Dec 3, 2015 · 23 comments
Closed

PLIP: Meta bundles generation #1277

ebrehault opened this issue Dec 3, 2015 · 23 comments

Comments

@ebrehault
Copy link
Member

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:

  • When we develop an add-on, it should be easy to declare our specific bundles so they are automatically included in such or such meta bundle.
  • When we install an add-on in our site, it should not require to compile bundles or other stuffs.

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:

  • default (loaded everytime)
  • logged-in (loaded if the user is logged in)

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:

  • If merge_with is defined, the bundle is aggregated in the corresponding meta bundle, and its expression property will be ignored.
  • If merge_with is nothing, the bundle is published directly in the site pages (just like it does at the moment) and the expression property is applied.
  • in Development mode, meta bundles are not published, all bundles are published directly.

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

  • a new version of Products.CMFPlone,
  • documentation.

Risks

N/A

Participants

Eric Bréhault

@ebrehault ebrehault added this to the Future milestone Dec 3, 2015
@jensens
Copy link
Member

jensens commented Dec 3, 2015

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

@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
Copy link
Member

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

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

jensens commented Dec 3, 2015

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

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

Nah.

@jensens
Copy link
Member

jensens commented Dec 3, 2015

:-D

@thet
Copy link
Member

thet commented Dec 3, 2015

What about this:

  • We name the attribute merge_with.
  • We concatenate bundles always on Zope startup (like it was done in Plone < 5, IIRC) or once after a GenericSetup profile import was done, if this was modifying the resource registry.

@mauritsvanrees
Copy link
Member

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

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

@ebrehault
Copy link
Member Author

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

+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
Copy link
Contributor

MrTango commented Dec 4, 2015

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

@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
Copy link
Contributor

frisi commented Dec 21, 2015

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) #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 plone/documentation#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 #1210)

@ebrehault
Copy link
Member Author

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

@frisi, sorry I told you wrong: what @vangheem changed is the stub_js_modules attribute (not the depend attribute), see #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
Copy link
Member Author

@bloodbare
Copy link
Member

Great work !

@yurj
Copy link
Contributor

yurj commented Jan 18, 2016

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

@ebrehault
Copy link
Member Author

@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
Copy link
Member

thet commented Jan 18, 2016

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

thet commented Jan 19, 2016

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants