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

Bandwidth Considerations #4

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jpdevries
Copy link
Collaborator

@jpdevries jpdevries commented Nov 3, 2016

View Markdown preview online

Voting

Only MODX Board Members may cast a vote. The Chief Architect and Chief Maintainer of the MODX Leadership are also granted one vote each. Votes may not be edited after they have been cast.

More info can be found in the voting protocol.

@OptimusCrime
Copy link

I am not an expert at this, but could the following be an option:

  • Use front end building tools, like Gulp, Webpack or friends.
  • Build ALL the JavaScript into one bundle.
  • Serve the bundle once with a name describing the current MODX version.
  • Set a high cache TTL value.
  • For a new version of MODX, the script name is altered.

This would:

  • Avoid white screen of death, as the JavaScript bundle is a part of the build process, which reports errors if any.
  • We could serve the entire bundle ONCE, and potentially never again.
  • With a new MODX version and a new named bundle script, it could unnecessarily load the script again, but we are certain that the users are not sitting on an outdated version of the bundle locally.

I've seen this pattern implemented many places. While relying on CDN cache where possible is a good idea, we should concatenate the internal MODX scripts as much as we can.

@jpdevries
Copy link
Collaborator Author

Use front end building tools, like Gulp, Webpack or friends.

We use Grunt now and I'd love to add Webpack.

Build ALL the JavaScript into one bundle.

This comes from the HTTP/1 days where optimized webpages by saving HTTP request. The Manager Interface is not a webpage, it is a web app. To effectively leverage the browser cache and optimize time-till-interactive on each page, we should avoid bundling. Serve lots of files, just the ones you use. Don't bundle. With HTTP/2 it can actually be faster to split them up then to pack everything together. This PR is somewhat related in that it proves how 2.0-2.4's use of the /min pattern was an anti–optimization (regardless of HTTP/2).

Once a file is loaded once, the service worker will cache it for offline support.

Serve the bundle once with a name describing the current MODX version.

Yes, cache busting is good. Also need to do the same with the service worker.

Set a high cache TTL value

Can we control that on their server or is it something we should just recommend? Either way, even if the server isn't configured for a high TTL the service worker will hold onto the file until it is updated.

Avoid white screen of death, as the JavaScript bundle is a part of the build process, which reports errors if any

👍 This was actually fixed in 2.5 for the most part modxcms/revolution#12611

We could serve the entire bundle ONCE, and potentially never again

Service workers, but we serve files individually and potentially never again.

With a new MODX version and a new named bundle script, it could unnecessarily load the script again, but we are certain that the users are not sitting on an outdated version of the bundle locally

Perhaps if we encourage Extra Developers to cache bust their assets based on the version of their package, and then we follow those same guidelines for "packages" including in the core we could solve this by more effectively leveraging the cache. Files that didn't change wouldn't get unnecessarily purged on a patch update this way.

While relying on CDN cache where possible is a good idea, we should concatenate the internal MODX scripts as much as we can

Concatenation is an anti-pattern for web apps like MODX. We should minify, GZIP, and use service workers for cache and offline support.

While relying on CDN cache where possible is a good idea

For things like React, jQuery I think we should encourage Extra Developers to:

  • detect if a sufficient version is already loaded
  • if not:
    • reach out to a common CDN
    • if the CDN fails load a local copy

That solves the 5 versions of jQuery on the same Manager page problem. It also means if you log into 15 MODX sites in a day, libraries and frameworks are only downloaded once (from a CDN and held in the browser cache). If we detect if the CDN failed and load a local copy which is cached by the service worker for offline support, we could support offline mode even with assets that are primarily loaded from a CDN. This could work even if that local copy hasn't been requested because the service worker could glutinously prime the offline cache for those local fallbacks in case they are ever needed and the CDN is unreachable because of a loss of or drop in network connectivity, firewall, etc.

@OptimusCrime
Copy link

Thank you for a good answer. I see you are relying a lot on HTTP/2 in the proposed recommendation. However, HTTP/1.1 has still around a 90% market share. With so much work going into making the manager lighter for "most people", would it not make sense to build this around the protocol that is used the most? While I agree that HTTP/2 is awesome and makes it easier to do what you want, following the HTTP/2 standard also means that the people stuck on HTTP/1.1 have to pay a price.

## Goals of Recommendation


To make the MODX Next management experience more accessible and less expensive for our global user base. To optimize the page load times of the MODX Next manager across the web as a whole. To promote stability of the MODX Next manager experience. To add costly features and enhancements in a prgoressive manor so they can be opted out of.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo prgoressive manor should be progressive manner?



## Relevant Recommendations
As bandwidth considerations is part of accessibility best practices, [DRAFT] Accessibility Mandate. This recommendation is proposed as a guideline accepted by the MODX Advisory Board and enforced by the Accessibility Mandate.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is part of => are part of

This recommendation is proposed as a guideline accepted by the MODX Advisory Board is redundant; you just described what a recommendation is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i just took that from Jason's template.

Leverage the browser cache and stability of the progressively enhanced manager with the following practices:


Front End (CSS, JS) assets delivered with the core shall not be compressed or concatenated on the server side.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be clarified to mean that they are not compressed or concat'd dynamically?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Point I was trying to make is that we probably don't want to flat-out decline compression if it's done in a responsible matter, like in a build step.

Copy link
Collaborator Author

@jpdevries jpdevries Nov 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is front end assets shouldn't be compressed on the server side. That should be done in a build process on the front end before checking in the code. Not in real time on the server (fingers crossed).

You have 0% code coverage if you are shipping code you concat & minify on the server especially with something like MODX installed on so many different server environments.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

Front End (CSS, JS) assets delivered with the core shall not be compressed or concatenated on the server side.
Front End assets delivered with the core shall first perform feature detection to test whether or not they are supported and not be loaded if they do not support the user’s environment.
Common Dependencies used by the core such as jQuery, React, AngularJS, etc shall conditionally be loaded from a common CDN with local fallbacks to preserve stability in the event the CDN is unreachable.
Extra Developers shall be encouraged, but not required, to perform feature detection to determine if a given asset has already been loaded.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably refer to it as Third Party Developers instead of Extra Developers as the term Extra is rather confusing in some contexts.

Take out but not required - that's already implied by being encouraged.

Server Admins will be encouraged to optimize the delivery of MODX by enabling HTTPS (HTTP/2), GZIP compression, and far future reaching expiry dates


A great deal of research, testing, and debate went into the changes introduced in 2.5 that improved the stability and performance of the delivery of front end dependencies. The TL;DR is compressing and concatenating scripts on the server side is an anti optimization. This worst practice made the Manager experience less stable, as it was the cause of dozens of “white screen of death” issues.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering what this adds to the recommendation. Perhaps turn it into something like

For an extensive discussion on the merit of dynamic compression, see this pull request for MODX 2.5.

as a note on the actual guideline in the list earlier, with a link to the changes we (you) made back then.

## Considerations
The cost of bandwidth varies greatly in different countries. For example, the average Brazilian works over 34 hours for every 500MB of bandwidth. By being considerate of bandwidth, we make the MODX site building more accessible to our global community.

![](http://j4p.us/0S0e2g1F2r0v/Image%202016-11-03%20at%2010.46.24%20PM.jpg)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think recommendations should contain an image hosted elsewhere, plus the data in that image is not accessible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub copies the images to their own CDN if you are worried about losing the image if the original goes away. I'm fine with adding alt text.




## Proof of Concept
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further to another comment, this doesn't necessarily recommend something new. It's worthwhile research to prove a point that could be linked to, but I'm not sure if this should be in the actual recommendation. Perhaps a simple proof of concept github repo people can interact with instead?

One reason I'd personally challenge putting too much original "research" into a recommendation, is that the research may change over time. Better examples, a live concept someone can actually try out, or perhaps something new comes out that changes the research. A recommendation will be pretty much static once accepted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with taking research out of the recommendation. It can be shared with the MAB without being in the recommendation itself. I wouldn't worry to much about anything changing this specific research though. The fundamentals of how browsers cache files (by the URL) hasn't changed and probably never will.



## Proof of Concept
Seen as how the pre-2.5 `compress_js` codebase was made under the assumption that developers should reduce the number of HTTP requests at all costs, and nothing else, it was frustrating to be asked to show numbers to prove that this was anti-optimal. That said, some interesting discoveries came from this process. For example, imagine this scenario:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I hurt your feelings by being stubborn about wanting proof back then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not about my feelings at all. My feelings weren't hurt. It was frustrating though. The reason I think this is something worth pointing out is the poor practice in question had not been tested or proven yet hard evidence was required to remove it. It took a lot of effort to prove to integrators the fundamentals of the cache; that browsers cache files by the URL.

I wouldn't expect any other contributor to have invested that much energy in making a project realize why their unproven and untested poor practice needed to be changed. So I think that if we are going to demand proof of contributors we should also be willing to prove and test the code we are arguing shouldn't be changed. Prove why it should be there in the first place, meet the contributor half way. Otherwise all the burden falls on the contributor and they may not be willing to make that significant of an effort.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In retrospective I definitely was too stubborn on that issue, and you're right that not a lot of contributors would go to the lengths you did. I'm glad you made the effort to convince me and have tried to keep a more open mind since ;)

@jpdevries
Copy link
Collaborator Author

However, HTTP/1.1 has still around a 90% market share.

This targets the first non-ExtJS Manager. By the time we are putting a bow on that and shipping it I'm confident HTTP/2 will be much more popular. IMO we should be writing and coding for the future, not today or the past. Also, service workers and HTTP/2 only work over HTTPS so we should probably encourage people to host the Manager over HTTPS.

While I agree that HTTP/2 is awesome and makes it easier to do what you want, following the HTTP/2 standard also means that the people stuck on HTTP/1.1 have to pay a price.

You can drop HTTP/2 from the picture and the practices explained here still hold up. Bundling everything would mean less HTTP requests an theoretically a faster page load the first visit. See modxcms/revolution#12611 (comment) this was tested with HTTP/1.

You should optimize web pages for first time visits. You should not optimize web apps for first time visits. A web app is something people click around on and use for a while. To bundle everything means sending unnecessary bytes unless the page in question uses all the scripts in that bundle. It also means wasted time and CPU executing those unneeded scripts.

I completely agree we don't want to make even HTTP/1 users "pay a price" which is exactly why we shouldn't bundle stuff together in a big file. Why make them pay for 2MB of script when the page they are on uses 1/10 of that? I understand these things are counter intuitive. Cache is weird.

Imagine this. We bundle 10 JS files into 1. It weights 1MB for easy math. Each JS file weights 100kB. We make one change by removing 1 line of code from one of those files. With bundling, the user spends the entire 1MB by having to re-download all those files. By not bundling, the cost is 100kB.

Most Manager installs I come across aren't GZIPing assets and don't have far future reaching expiry dates. GZIP can reduce file sizes by 10x. Cached files bypass a request entirely. There are many things, such as these, to consider before being concerned with the number of HTTP requests a web app makes.

@OptimusCrime
Copy link

This targets the first non-ExtJS Manager.

Oh, okey. Is there a way to distinguish what version or future milestone the recommendations are targeting? Because, as far as I've understood, MODX Revo 3.0 will still have the same ExtJS as a part of the Manager core, to support as many extras as possible.

@jpdevries
Copy link
Collaborator Author

Oh, okey. Is there a way to distinguish what version or future milestone the recommendations are targeting? Because, as far as I've understood, MODX Revo 3.0 will still have the same ExtJS as a part of the Manager core, to support as many extras as possible.

That's a great question. Part of the reason I'm just saying "the first non-ExtJS" is I'm not entirely sure when that will be. We actually just discussed this today on the MAB call and it sounds like 3.0 will basically be 2.x + composer and then we'll make breaking changes with the Manager interface in 4.0.

Don't let that bum you out too much though. We aren't at all opposed to a 4.0 alpha dropping soon after 3.0 is released.

@OptimusCrime
Copy link

Makes sense. That clears up some questions about this + other recommendations. I think it would be easier to understand what the recommendation tries to do, and when, if it had a target version/milestone. The PHP RFCs have these, following semver.

@Mark-H Mark-H added the Draft label Nov 24, 2016
@jpdevries
Copy link
Collaborator Author

Just came across this in WDRL 160

Firefox will soon warn people if they are about to enter a password in insecure (HTTP) contexts. It’s time to secure every login with an HTTPS connection and enforce this as a best practice

https://mobile.twitter.com/ryanfeeley/status/801539237682302987
🙀🔐

With so many browsers offering incentives to move to HTTPS, and even them warnings on sites that aren't HTTPS, along with services like letsencrypt, I think it safe to say that's the way the web is heading. MODX Managers hosted over HTTPS can benefit from HTTP/2, offline support (service workers), and obviously increased security.

@OptimusCrime
Copy link

Sure, but serving all as HTTPS does not in any way require HTTP/2. It will be interesting to see how quickly host providers pick up on HTTP/2.

@jpdevries
Copy link
Collaborator Author

Sure, but serving all as HTTPS does not in any way require HTTP/2

My point is that, even though the spec isn't explicit, all browsers require HTTPS for HTTP/2. So they go hand in hand. Movement towards one is movement towards the other. They are meant to incentivize each other.

@jpdevries
Copy link
Collaborator Author

Some more context on MODX and HTTPS
https://modx.com/blog/https-and-secure-websites-matter-for-everyone

TL;DR

  • HTTP warnings have arrived

@jpdevries
Copy link
Collaborator Author

With Chrome 58 HTTPS is required for Web Notificafions API, further incentivizing web apps to make the switch.

@jpdevries
Copy link
Collaborator Author

Thinking maybe this could use a clause on code splitting / lazy loading. The idea that cropping code shouldn't be downloaded until the cropping modal is opened up for example. This pattern can be accomplished with ease using Webpack and ES7 async/await

@OptimusCrime
Copy link

OptimusCrime commented Jun 5, 2017

@jpdevries
Copy link
Collaborator Author

Interesting read @OptimusCrime. I think we will definitely want code splitting. It is very easy to do with Webpack and really helps with lazy loading. And the MODX Manager is probably more of a web app than a library, so I'd lean towards using Webpack.

@jpdevries
Copy link
Collaborator Author

Pulled the history about 2.5 out, leaving it here as a comment for history

A great deal of research, testing, and debate went into the changes introduced in 2.5 that improved the stability and performance of the delivery of front end dependencies. The TL;DR is compressing and concatenating scripts on the server side is an anti optimization. This worst practice made the Manager experience less stable, as it was the cause of dozens of “white screen of death” issues.

The MODX Manager is not a website! The assets it loads vary from page to page, user to user, and change as Extras are enabled, installed, and disabled. From a bandwidth perspective, the Manager is the opposite of a web site. As proven in the Proof of Concept section, optimization that commonly apply to websites are anti-optimal when applied to the manager experience.

Proof of Concept

Seen as how the pre-2.5 compress_js codebase was made under the assumption that developers should reduce the number of HTTP requests at all costs, and nothing else, it was frustrating to be asked to show numbers to prove that this was anti-optimal. That said, some interesting discoveries came from this process. For example, imagine this scenario:

  • Let's say we have 350kb of scripts that when server side compress_js is used is all in a URL that contains a comma separated list of dependencies like this:
    /manager/min/index.php?f=/manager/assets/modext/core/modx.localization.js,/manager/assets/modext/util/utilities.js
  • Everything is instead in it's own file and possibly not uglified at all (compress_js disabled)

With these user stories:

  • compress_js enabled
  • User visits a Resource edit page with compress_js enabled
  • We send them 350kb on script in a single file. redactor.js is one of the included files
  • User disabled Rich Text on the resource and refreshes the page
  • New URL, so we send them all the scripts again and leverage the cache for none of them even though all we are doing is sending them same scripts minus redactor

• User visits a Resource edit page with compress_js disabled
• We send them 350kb or more of script in individual files. redactor.js is one of the included files
• User disabled Rich Text on the resource and refreshes the page

Scenario A has a cache usage of 0%. Even though we are serving less data by disabling an Extra we resend all the bytes. The URL changes, the cache is purged. We couldn’t leverage the browser cache less if we tried. We couldn’t make the experience more expensive in terms of bandwidth if we tried.

Scenario B has a cache usage of 100%. Each dependency is its own request, and therefore own URL. The cache is leveraged as much as possible. Yes this means more HTTP requests and a possibly slower load time (probably not) over HTTP/1 but that only applies to the first page load. Once assets are cached, they are cached!

@jpdevries
Copy link
Collaborator Author

I think this is ready for review now. Updated the label.

@OptimusCrime
Copy link

Can this PR be closed or merged? It keeps appearing in my "Pull requests" overview, and I can not see any way of removing it myself. Thanks.

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

Successfully merging this pull request may close these issues.

3 participants