Skip to content

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented Mar 22, 2017

Simple PoC for the JSCombiner

current situation: We load a crapload of javascript files. Often an app will always loads the same files. And because we are not animals we split code between multiple files. However this leads to poor utilisation

This PoC shows the JSCombiner (pattent pending). Developers can create a json file that just lists all the files that can be combined (in order). Then the process is basically similar to the way we handle combining of SCSS files.

This means we can start combining js files to significantly reduce the number of requests on page load.

TODO:

  • Implement proper caching
  • Implement debug mode (just load the files then)
  • Tests

@mention-bot
Copy link

@rullzer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @LukasReschke and @bartv2 to be potential reviewers.

@rullzer rullzer force-pushed the jscombiner branch 3 times, most recently from 30e787f to a033cef Compare March 22, 2017 13:51
rullzer added 6 commits March 24, 2017 10:58
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@codecov-io
Copy link

Codecov Report

Merging #3988 into master will increase coverage by <.01%.
The diff coverage is 58.4%.

@@             Coverage Diff              @@
##             master    #3988      +/-   ##
============================================
+ Coverage     54.23%   54.24%   +<.01%     
- Complexity    21231    21272      +41     
============================================
  Files          1308     1310       +2     
  Lines         81080    81214     +134     
  Branches       1284     1284              
============================================
+ Hits          43974    44051      +77     
- Misses        37106    37163      +57
Impacted Files Coverage Δ Complexity Δ
lib/private/TemplateLayout.php 0% <0%> (ø) 34 <0> (ø) ⬇️
core/routes.php 0% <0%> (ø) 0 <0> (ø) ⬇️
lib/private/Template/JSResourceLocator.php 0% <0%> (ø) 20 <5> (+8) ⬆️
core/Application.php 0% <0%> (ø) 2 <1> (ø) ⬇️
apps/files_sharing/appinfo/app.php 43.9% <0%> (+2.04%) 0 <0> (ø) ⬇️
lib/private/Template/JSCombiner.php 71.79% <71.79%> (ø) 20 <20> (?)
core/Controller/JsController.php 94.44% <94.44%> (ø) 3 <3> (?)
apps/comments/lib/EventHandler.php 79.16% <0%> (-8.34%) 7% <0%> (ø)
.../federation/lib/Middleware/AddServerMiddleware.php 86.66% <0%> (-5.65%) 4% <0%> (+1%)
apps/dav/lib/CardDAV/SyncService.php 44.16% <0%> (-2.69%) 34% <0%> (+6%)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35d3a08...be6acbe. Read the comment docs.

@rullzer rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 24, 2017
@rullzer
Copy link
Member Author

rullzer commented Mar 24, 2017

Ready for review!

@michaelletzgus
Copy link
Contributor

michaelletzgus commented Mar 24, 2017

I checked out the jscombiner branch - but there is no visible difference to master when loading the files pages. Both perform 198 requests.

What am I missing? I guess I have to define which js files should be aggregated somewhere? But how?
Like the json file in the tests folder?

@MorrisJobke
Copy link
Member

What am I missing? I guess I have to define which js files should be aggregated somewhere? But how?
Like the json file in the tests folder?

As this is a PoC - Proof of Concept - for now only 3 files are combined into one. Be patient we are working on it.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Code looks good and works 👍

@MorrisJobke
Copy link
Member

What am I missing? I guess I have to define which js files should be aggregated somewhere? But how?
Like the json file in the tests folder?

We will add more combined files once we have this merged. Then we can easily add more combined stuff step by step.

@LukasReschke LukasReschke merged commit 2cd79a8 into master Mar 24, 2017
@LukasReschke LukasReschke deleted the jscombiner branch March 24, 2017 18:31
@LukasReschke
Copy link
Member

Follow-up for the file list at #4038, removes 26 requests.

@LukasReschke
Copy link
Member

Follow-up for comments at #4039, removes 6 requests.

@LukasReschke
Copy link
Member

Follow-up for files_versions at #4040, removes 3 requests.

@LukasReschke
Copy link
Member

Follow-up for systemtags at #4041, removes 7 requests.

@ChristophWurst
Copy link
Member

Good job, this is awesome 🚀

@LukasReschke
Copy link
Member

Follow-up for sharing backend JS at #4042, removes 8 requests.

@LukasReschke
Copy link
Member

Follow-up for template prepend core JS at #4043, removes 14 requests.

@LukasReschke
Copy link
Member

Follow-up for notification JS at nextcloud/notifications#66, removes 2 requests.

@LukasReschke
Copy link
Member

Follow-up for files_texteditor at nextcloud/files_texteditor#32, removes 2 requests.

@LukasReschke
Copy link
Member

Follow-up for gallery at nextcloud/gallery#225, removes 31 requests.

@LukasReschke LukasReschke mentioned this pull request Mar 24, 2017
@LukasReschke
Copy link
Member

Follow-up for login view at #4044, removes 2 requests.

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

Labels

3. to review Waiting for reviews enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants