Ace ventura
What does this MR do?
Makes Ace only load when it's actually necessary. The only two places it seems to be used is for Snippets and the File (Blob) Editor.
Before minification or compression, this takes the application.js down from 2.2MB to 1.6MB, and also removes two inline scripts :D
Compressed/Minified: master: 317KB/1.1MB, ace-ventura: 220KB/771KB
Are there points in the code the reviewer needs to double check?
That this doesn't just completely break anything.
Why was this MR needed?
Ace is a big library, this allows us to only load it when necessary.
What are the relevant issue numbers?
Does this MR meet the acceptance criteria?
-
CHANGELOG entry added - Tests
-
All builds are passing
-
-
Conform by the style guides -
Branch has no merge conflicts with master
(if you do - rebase it please) -
Squashed related commits together
Merge request reports
Activity
Added 73 commits:
- c3a0a4d6...e45d6043 - 70 commits from branch
master
- 31673382 - Only load Ace on Snippets and file edit pages.
- 86092668 - Probably important to include the files, huh?
- 548fbe43 - Forgot one.
Toggle commit list- c3a0a4d6...e45d6043 - 70 commits from branch
@alfredo1 helped a ton with debugging this, he found that the version of PhantomJS we use is severely out-of-date (using a WebKit version from before 2012, at least). We should upgrade from 1.9.8 to 2.1.1 (latest). We'll have to build it ourselves, unfortunately.
Edited by Connor Sheamentioned in issue #4216 (closed)
Added 1 commit:
- 06bdce74 - Testing apt-get for installing phantom.
Added 1 commit:
- ffd1c0df - Testing apt-get for installing phantom.
Added 1 commit:
- d694f60d - Reverting.
Blocked by #4216 (closed)
@ayufan can we update PhantomJS? Pretty please
@jschatz1 Yes, can we do that after 8.10 release?
@ayufan I'd prefer to get this MR in this release, but if you're short on time then delaying it is perfectly fine.
Alternatively, do you know if anyone else would be able to do this for us?
mentioned in issue #19866 (closed)
@connorshea Lets hold our breath to have this merged: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5279 :)
@ayufan it's been merged! :D
@connorshea Yes, but just because it's merged it doesn't mean that it's enabled on GitLab.com. I have call about this with Pablo on Wednesday to came up with migration plan
mentioned in merge request gitlab-com/www-gitlab-com!2497 (merged)
Milestone changed to %8.11
mentioned in merge request !5501 (merged)
Added 2031 commits:
-
d694f60d...a77394a7 - 2028 commits from branch
master
- 92827930 - Only load Ace on Snippets and file edit pages.
- 7fb27b30 - Probably important to include the files, huh?
- 98738303 - Update to use not-coffee.
Toggle commit list-
d694f60d...a77394a7 - 2028 commits from branch
Added 1 commit:
- acf61026 - Remove CoffeeScript.
Added 1 commit:
- c36e8955 - Only load Ace on Snippets and file edit pages.
Added 400 commits:
-
c36e8955...7dde4ed2 - 399 commits from branch
master
- f6f8275b - Only load Ace on Snippets and file edit pages.
-
c36e8955...7dde4ed2 - 399 commits from branch
Reassigned to @alfredo1
Marked the task Conform by the style guides as completed
Marked the task Squashed related commits together as completed
Reassigned to @jschatz1
@jschatz1 Just tested and everything works fine, 650KB saved in development mode, a little under 100KB in production. Merge? :D
Added 343 commits:
-
f6f8275b...3666f698 - 342 commits from branch
master
- 882dae6e - Only load Ace on Snippets and file edit pages.
-
f6f8275b...3666f698 - 342 commits from branch
@connorshea builds failing.
@connorshea If we can get the builds passing then we should be good.
Added 60 commits:
-
882dae6e...ac73de50 - 59 commits from branch
master
- 36ecc470 - Only load Ace on Snippets and file edit pages.
-
882dae6e...ac73de50 - 59 commits from branch
Added 1 commit:
- 532a9379 - Don't use global variables.
Reassigned to @tauriedavis
Reassigned to @connorshea
@connorshea let me know once builds pass.
Added 1 commit:
- 5cb96c02 - Blob has to be global, unfortunately.
Added 1 commit:
- c5189783 - Don't use global variables.
Added 1 commit:
- 415733e2 - Don't use global variables.
Added 59 commits:
-
415733e2...d97c8309 - 57 commits from branch
master
- 0baaf490 - Only load Ace on Snippets and file edit pages.
- 8db088cc - Don't use global variables.
-
415733e2...d97c8309 - 57 commits from branch
44 44 end 45 45 46 46 step 'I should see its content with new lines preserved at end of file' do 47 expect(evaluate_script('blob.editor.getValue()')).to eq "Sample\n\n\n" Reassigned to @jschatz1
Builds are passing now
@jschatz116 19 = link_to '#preview', 'data-preview-url' => namespace_project_preview_blob_path(@project.namespace, @project, @id) do 17 20 = editing_preview_title(@blob.name) 18 21 19 = form_tag(namespace_project_update_blob_path(@project.namespace, @project, @id), method: :put, class: 'form-horizontal js-quick-submit js-requires-input js-edit-blob-form') do 22 = form_tag(namespace_project_update_blob_path(@project.namespace, @project, @id), method: :put, class: 'form-horizontal js-quick-submit js-requires-input js-edit-blob-form', data: {'relative-url-root' => "#{Rails.application.config.relative_url_root}", 'assets-prefix' => "#{Gitlab::Application.config.assets.prefix}", 'blob-language' => "#{@blob.language.try(:ace_mode)}" }) do @stanhu done!
Reassigned to @smcgivern
@smcgivern Would love your review on this too.
Added 1 commit:
- 9cf6d451 - Move editor paths to helper
I tested this locally with precompiled assets in dev mode, and yep, it works great for both editing existing files and creating new ones! Syntax highlighting modes are still applied, and we save the 100-odd KB from our application.js when you aren't editing a file.
Awesome work @connorshea, and good patience too
Reassigned to @jschatz1
Enabled an automatic merge when the build for 9cf6d451 succeeds
mentioned in commit 8fc800cb
mentioned in merge request !5909 (merged)
mentioned in commit 310eef45
mentioned in commit d53ef2e8
mentioned in commit 4030d5ab
mentioned in merge request gitlab-com/www-gitlab-com!2910 (merged)