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

Remove get_scripts wrapper to download javascript #13839

Merged
merged 4 commits into from Nov 29, 2017

Conversation

Projects
None yet
1 participant
@nijel
Member

nijel commented Nov 28, 2017

Similarly like in #13797, I think this no longer has reasonable benefits.

Before submitting pull request, please check that every commit:

  • Has proper Signed-Off-By
  • Has commit message which describes it
  • Is needed on it's own, if you have just minor fixes to previous commits, you can squash them
  • Any new functionality is covered by tests

@nijel nijel added this to the 4.8.0 milestone Nov 28, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 28, 2017

Codecov Report

Merging #13839 into master will increase coverage by 0.02%.
The diff coverage is 47.36%.

@@            Coverage Diff             @@
##           master   #13839      +/-   ##
==========================================
+ Coverage   53.88%    53.9%   +0.02%     
==========================================
  Files         493      492       -1     
  Lines       80982    80875     -107     
==========================================
- Hits        43634    43598      -36     
+ Misses      37348    37277      -71

codecov bot commented Nov 28, 2017

Codecov Report

Merging #13839 into master will increase coverage by 0.02%.
The diff coverage is 47.36%.

@@            Coverage Diff             @@
##           master   #13839      +/-   ##
==========================================
+ Coverage   53.88%    53.9%   +0.02%     
==========================================
  Files         493      492       -1     
  Lines       80982    80875     -107     
==========================================
- Hits        43634    43598      -36     
+ Misses      37348    37277      -71

nijel added some commits Nov 28, 2017

Remove usage of get_scripts.js.php wrapper
- this adds significant overhead in sending static files from the server
- the benefit of joining requests is minimal since introduction of HTTP/2
- the onload handlers are now fired directly by individual scripts, what
  makes them really execute when the script is loaded (it could be
  previously executed earlier in case of chunked loading)

Fixes #13840

Signed-off-by: Michal Čihař <michal@cihar.com>
Remove line counts logic from the error reporting component
We no longer need this as we download js files directly, so the client
has correct filename and line immediatelly.

Signed-off-by: Michal Čihař <michal@cihar.com>
Add missing dependency on jquery.validate.js
Signed-off-by: Michal Čihař <michal@cihar.com>
Simplify Scripts::addFile API
Removed unused parameter to define ordering of scripts.

Signed-off-by: Michal Čihař <michal@cihar.com>

@nijel nijel changed the title from RFC: Remove get_scripts wrapper to download javascript to Remove get_scripts wrapper to download javascript Nov 29, 2017

@nijel nijel merged commit 7ec4059 into phpmyadmin:master Nov 29, 2017

5 of 6 checks passed

codecov/patch 47.36% of diff hit (target 53.88%)
Details
DCO All commits have a DCO sign-off from the author
Scrutinizer Analysis: 3 new issues, 6 updated code elements – Tests: passed
Details
codacy/pr Good work! A positive pull request.
Details
codecov/project 53.9% (+0.02%) compared to 6a0345b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nijel nijel deleted the nijel:scripts branch Nov 29, 2017

@nijel nijel self-assigned this Nov 29, 2017

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