magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.48k stars 9.29k forks source link

HTML/JS requests (was: Remove inline scripts to improve page rendering speed) #155

Closed blitux closed 10 years ago

blitux commented 11 years ago

Hi team,

Magento 1.x actually has a lot of inline scripts to enable dynamic behaviour on forms, validation, autocompletion, etc. This has a negative effect on page rendering speed because, as you know, web browsers stop page render when they parse javascript.

A common solution for this issue is to put all js code before the </body> tag, to enable the browser to render the whole page prior to executing javascript code and thus improving the perceived loading speed. But this is actually impossible because most inline scripts have dependencies and need prototype or some other scripts to run properly.

Also, I've seen prototype is still shipped with magento 2. It would be really nice to reduce to the minimum possible the javascript files to also improve download time.

OT: I also have another requests, but I don't know if you're working on it or if they have been planned, so I'm asking for an advice on where to leave those petitions.

Thank you for your time!

magento-team commented 11 years ago

Hi. There are still significant changes coming to Magento in the JS part so it will be differerent (and prototype should be removed). Please leave the other request in this thread.

blitux commented 11 years ago

Hi team,

I'll be leaving all the requests here. Should I change the title? Some of the requests are taken from Drupal 8 Initiative. I know @mage2-team will decide what and how to implement this issues, I'm just leaving some requests that I think need to be addressed, issues I was finding while I was developing a couple og Magento stores. Most of them are frontend related (I mean what is being sent to the web browser). Sorry for my basic english skills :)

Javascript

HTML & Templates

Other Issues

I've encountered some other issues while I was developing magento sites:

Upgrade Path: It was almost imposible to upgrade Magento using an existing database. Upgrading leads to an unusable magento site. Most of the time I had to export products, losing customer's history data.

Import/Export: there is no easy way to export products/categories/customer data and importing it again on a new store.

Resources: Magento consumes a lot of resources, it's almost impossible to make it run on a shared hosting, needing a VPS at least to run decently.

Modules: a lot of magento modules are poorly coded and don't follow code standards. Also there are no guidelines on where to put menu entries, leading to inconsistencies (some modules place menu entries on main navigation menu, some others inside configuration, etc.). Would be nice to rework module folder structure. In Magento 1.x modules are including files that are being placed on /skin, /app/code/local or /app/code/community and /app/design/frontend/base or /app/design/frontend/default.

Thank you for your time!

Sources:

Principles of Good Design http://drupal.org/node/1087784#comment-4286614 HTML5 Design Initiative http://drupal.org/community-initiatives/drupal-core/html5 Configuration Management http://groups.drupal.org/node/155559

magento-team commented 11 years ago

Hi Blitux,

Please find my answers below what is planned and what is not. Please note that priorities might change so this is not final yet.

Javascript

Remove inline scripts to improve rendering speed.-> planned to reduce inline scripts, not remove Reduce js dependencies to improve rendering speed and download time.-> in progress, communication via events Move js before body tag. ->We load jQuery, jQuery-UI and our base framework mage.js in head, all other scripts get loaded for each component (widget/plugin) when it get instantiated.

HTML & Templates

Implement Full HTML5 support.-> planned JavaScript fallback (HTML5 shiv) for HTML5 elements in non-supported browsers -> planned Optimize the html markup of the template files. -> planned Reduce and modularize CSS files. -> planned Implement a template engine to allow non developers to write themes for Magento -> not planned in near timeframe New HTML5 theme for the backend -> planned Provide a blank default HTML5 base theme -> under consideration

CodeMonkey90 commented 11 years ago

Are you sure you won't be removing the remaining inline scripts? I don't think there's any justification for them (laziness doesn't count) and removing them would enable us to send a strict Content-Security-Policy header, which is a nice security measure against XSS attacks.

verklov commented 10 years ago

@Landkeks , as noted previously, we do not have removal of the remaining inline scripts in our plans.

blitux commented 10 years ago

It's ok. Just want to note that inline scripts will block rendering making any template/skin load not as fast as it would be without them. A good refactoring of JS side would be needed, and sincerely I don't have any idea of what would be involved in that task.

brendanfalkowski commented 10 years ago

It's very disappointing that Magento is not motivated to remove inline scripts from templates. Even four years ago this would be disallowed in a modern JS architecture. By continuing this pattern, you're inviting all 3rd party developers to latch onto this bad practice again, and lower the performance bar for the frontend (which is still very low for Magento).

Related, it's even more upsetting to hear that jQuery UI is being bundled as a dependency. This adds bloated and over-configurable components that could easily be written in a small amount of native JS or basic jQuery.

I don't mean to overly critical, but my overall impression of the frontend engineering in Magento 2 is that almost nothing has changed since Magento 1. Yes, Prototype has been swapped for jQuery, but that's really a sideways move at best. Many modern applications are built with library independence, which allows the implementor to choose a framework that best suits the project's needs.

It's great that the backend is getting a full rewrite, but disregarding practical, modern, and necessary improvements to the frontend (as per Magento 1.x) is not prudent. With responsive design being the only practical multi-device approach today, it's more important than ever to invest in frontend architecture. Modern frontends are being hired to do the job of multiple device-specific websites today.

My two cents: I don't see Magento making those investments.

SchumacherFM commented 10 years ago

:+1: for brendanf. I've managed to remove all inline JS in Magento 1.X even in the checkout ... hard work for great improvements

tim-bezhashvyly commented 10 years ago

Same thoughts about jQuery UI. Redundant dependancy.

If Magento has no initiative removing inline scripts or doing any other FE improvements they can pay more attention to pull requests made by community members.

bobbyshaw commented 10 years ago

Thanks @blitux for raising this issue. I agree wholeheartedly with @blitux and @brendanf here. We need some major frontend rearchitecting.

I've put similar efforts in to try and remove inline js from Magento 1.x because of the pain it causes when trying to optimise sites.

I hope that Magento can reconsider its viewpoint on the frontend improvements. I'm happy to chip in with suggestions, discussions and code commits also if they'd be found helpful.

benmarks commented 10 years ago

Perhaps some benchmarks from @brendanf and @bobbyshaw can make it easier for the core team to decide that this is a priority with the remaining time left.

blitux commented 10 years ago

I've done a Q&D research about inline scripts in Magento 1.x. Actually most of the scripts are trivial, doing things like decorate lists (things that should be done with CSS) init forms to leverage client side validation, and stuff like that. This is a quick list of inline scripts. Each line represents a <script> tag:

All Pages

Set cookies path and domain
Set optional zip countries
Search form init/autocomplete
Validate Poll Answer is selected
Decorate list poll answers

Categories Views

Decorate generic
Decorate data list
Newsletter subscriber form detail

Product View

Set optionsPrice
Init zoom on window load
Product add to cart form (submit) and validation
Decorate table product attribute specs
Decorate table upsell product table
Decorate generic product tags
addTagFormJs
Detting cookies expires
Decorate list block related
Add Related Products on click

And this is just these pages. This needs a serious refactoring, ie. instead of several inline scripts to leverage validation, add a function inside a common.js or mage.js or whatever.js that leverages validation on all forms. With this architechture, is impossible to move scripts to bottom, because the inline scripts have dependencies that should be loaded before they run. Discard all "decoration" scripts and replace them with css. I guess Magento have some legacy code from maybe 5 years or more that needs to be refactored or removed, removing some obsolete dependencies, etc.

Page load time is indeed a very important issue because we're not talking here about a blog or a corporate site, we're talking about bussiness. You lose money if your site take seconds to load, so the faster it loads, the more your business get.

As a side note, most of the extension producers writes code with LOTS of inline scripts, jquery dependencies being loaded twice or more with different versions, making practically IMPOSSIBLE to optimize. There are no conventions or good practices advices nor a guideline for writing frontend extensions. So, there's a lot of room for improvements here.

rgranadino commented 10 years ago

Benchmarks would depend on what's actually running in the script block(s). The browser will block the rendering/loading process of the page for each script tag while it interprets and executes the script content.

Here's an example of this behavior provided by PageSpeed documentation:

In-line scripts processed: http://www.modpagespeed.com/defer_javascript.html?ModPagespeed=off Script tags deferred: http://www.modpagespeed.com/defer_javascript.html?ModPagespeed=on&ModPagespeedFilters=defer_javascript

From: https://developers.google.com/speed/pagespeed/module/filter-js-defer

It'd be great to see Magento update this in the base/default theme and set a good example for developers (and not just put it aside for the sake of releasing something sooner).

brendanfalkowski commented 10 years ago

Magento needs an architectural target to have zero core JS in the <head>. There isn't a modern excuse for doing this, it's just lazy or ignorant. I hope it's ignorance.

As @blitux pointed out, all the inline decorateThing() scripts are completely irrelevant. I've been removing them since 2009 and using http://selectivizr.com/ to polyfill CSS3 selectors in older browsers.

All form validation could be triggered blindly based on form markup and data params being present.

All other inline JS defined in templates could be split off and assigned to a "JS roundup block" that is placed in "before_body_ends". That's the dirtiest way to clean up.

@rgranadino Setting defer or async attributes on script tags is still sub-optimal. The asset is still loaded simultaneously with the page content, which makes rendering slower. JS that should execute post-load anyway doesn't even need to be requested until the page is rendered. Since most JS falls on the non-critical pipeline this should be the default, and exceptions can be set otherwise. See: http://www.feedthebot.com/pagespeed/defer-loading-javascript.html

A truly modern approach is using asynchronous module definition (de facto: http://requirejs.org) to assert dependencies and perform loading. No JS assets would load in markup except for the AMD script, which uses a JSON configuration to pull required assets for that page. You could even store the responses in HTML5 localStorage.

This is the best blend of modular and cacheable JS architecture, and would move worlds to improve interoperability among Magento extensions scrambling to include multiple instances of jQuery.

The benchmark would be time-to-render between Magento 2 (as of now) with all its JS in the head, and having zero JS assets loaded in the DOM (because that's the effect AMD has on rendering performance). It's a no-brainer to me.

benmarks commented 10 years ago

@brendanf What's the level of effort for you to do this, if you wanted to and were given the beaucoup bucks it would take?

brendanfalkowski commented 10 years ago

It's high touch, because for AMD to have an impact it can't be piecemeal. The entire frontend needs to be enrolled before you can wipe out the worst practices. The status quo is do nothing, and that's the challenge. This is a people problem not a technology problem.

As I've said before: https://speakerdeck.com/brendanfalkowski/responsive-ecommerce-part-two?slide=82

rgranadino commented 10 years ago

@brendanf the example I posted is not related to the defer or async attributes on script tags. It's an example that demonstrates how page rendering is affecting when processing <script> tags in line vs at the end of the page (the mod_pagespeed module ends up changing the <script>'s type attribute and processing them on window.load). My main point is that I think it'd be difficult to compare apples to apples in magento without actually doing all the work involved.

I feel its a change that's important in order for magento2 to stay current in regards to following best practices. My guess is it will come down to cost. It will cost eBay/Magento Inc. time and development resources to implement this change and it might not be "worth" it to them to do so.

I believe that in the long run these types of decisions (zf2, twig, js/frontend architecure, etc) will end up "costing" them much more as they start to add up, but that's just me.

bobbyshaw commented 10 years ago

I think it's less to do with cost and more in terms of the right person/ expert pitching it. They spent the time and money on a complete redesign of the admin with no apparent gains from it and arguably worse from a mobile experience.

In the grand scheme of things the amount of effort it would to make it best practice isn't that much. There's a hackathon coming up, of they'd let us I'm sure there's a few of us that would get involved if we knew it was a direction they were happy to take.

On Wednesday, 15 January 2014, beeplogic wrote:

@brendanf https://github.com/brendanf the example I posted is not related to the defer or async attributes on script tags. It's an example that demonstrates how page rendering is affecting when processing Githubissues.

  • Githubissues is a development platform for aggregating issues.