Skip to content
Snippets Groups Projects

Remove IIFEs for several JS files - Part 1

Merged Remove IIFEs for several JS files - Part 1
remove-iifes-1 into master
2 unresolved threads
Merged username-removed-636429 requested to merge remove-iifes-1 into master
2 unresolved threads

What does this MR do?

removes the IIFEs wrapping several JS files... this is a necessary step toward moving to ES module exports because export default Foo is statically analyzed and cannot exist within an IIFE.

I've just started going through scripts alphabetically starting with abuse_reports.js and going through header.js. I'll open other MRs for additional segments of the frontend codebase.

Are there points in the code the reviewer needs to double check?

Keep in mind no code was changed here, only indentation level and IIFE removal. The diffs may look complicated because sometimes our diff method doesn't understand indentation changes. Occasionally, I need to change this to window or global to window.gl where appropriate, but that's about it.

Why was this MR needed?

In order to enable code splitting, we first need to get all of our script dependencies into statically analyzable chunks and IIFEs make that difficult.

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

#29219 (moved)

related to #27486 (moved)

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
app/assets/javascripts/autosave.js
49 };
50
51 Autosave.prototype.reset = function() {
52 if (window.localStorage == null) {
53 return;
54 }
55 try {
56 return window.localStorage.removeItem(this.key);
57 } catch (error) {}
58 };
59 59
60 return Autosave;
61 })();
62 }).call(window);
60 return Autosave;
61 })();
  • username-removed-408881
    username-removed-408881 @asumaran ·
    Contributor

    This still have IIFEs

  • username-removed-636429
    username-removed-636429 @mikegreiling ·
    Author Maintainer

    That's true, but in this case it is due to the class definition implementation generated by coffeescript. When we come back and refactor this one in another pass it will be a simple matter of changing:

    window.Autosave = (function() {
  function Autosave(field, key) { /* ... */ }
  Autosave.prototype.restore = function() { /* ... */ };
  Autosave.prototype.save = function() { /* ... */ };
  Autosave.prototype.reset = function() { /* ... */ };
  return Autosave;
})();

    to

    class Autosave {
  constructor(field, key) { /* ... */ }
  restore() { /* ... */ }
  save() { /* ... */ }
  reset() { /* ... */ }
}

    and all the indentation will be the same. Again this is just to make the future changes easier to review...

    Edited by username-removed-636429
  • username-removed-408881
    username-removed-408881 @asumaran ·
    Contributor

    In my MR I was removing all IIFEs and keeping the code exactly the same.

    Do you think this is worth it? Or we should translate all those CS generated to ES6 Classes instead?

  • username-removed-636429
    username-removed-636429 @mikegreiling ·
    Author Maintainer

    I think for cases like this, it is very easy to translate to ES6 classes. Those lines in my above example are literally the only ones that need to change, and I think the end result is a lot more readable.

  • username-removed-408881
    username-removed-408881 @asumaran ·
    Contributor

    Cool. I'll keep in mind.

  • Please register or sign in to reply
  • username-removed-408881
    username-removed-408881 @asumaran started a thread on the diff
    • app/assets/javascripts/autosave.js
    33 return this.field.trigger("input");
    34 };
    3 window.Autosave = (function() {
    4 function Autosave(field, key) {
    5 this.field = field;
    6 if (key.join != null) {
    7 key = key.join("/");
    8 }
    9 this.key = "autosave/" + key;
    10 this.field.data("autosave", this);
    11 this.restore();
    12 this.field.on("input", (function(_this) {
    13 return function() {
    14 return _this.save();
    15 };
    16 })(this));
  • username-removed-636429
    username-removed-636429 @mikegreiling ·
    Author Maintainer

    @alfredo1 since you've already started looking at this one, would you mind reviewing/merging it? It looks like it's close to passing CI...

    don't forget the ee-branch also (gitlab-ee!1415) :smiley:

  • username-removed-636429 assigned to @alfredo1

    assigned to @alfredo1

  • username-removed-408881
    username-removed-408881 @asumaran ·
    Contributor

    lol. I'm not sure why I started reviewing this before being assigned :D

  • username-removed-408881 Approved this merge request

    Approved this merge request

  • username-removed-636429 marked the task All builds are passing as completed

    marked the task All builds are passing as completed

  • username-removed-636429 marked the task Conform by the merge request performance guides as completed

    marked the task Conform by the merge request performance guides as completed

  • username-removed-636429 marked the task Conform by the style guides as completed

    marked the task Conform by the style guides as completed

  • username-removed-636429 marked the task Branch has no merge conflicts with master (if it does - rebase it please) as completed

    marked the task Branch has no merge conflicts with master (if it does - rebase it please) as completed

  • username-removed-636429 marked the task Squashed related commits together as completed

    marked the task Squashed related commits together as completed

  • username-removed-408881
    username-removed-408881 @asumaran ·
    Contributor

    @mikegreiling LGTM :ok_hand:

  • username-removed-408881 merged

    merged

  • username-removed-408881 mentioned in commit 302d0453

    mentioned in commit 302d0453

  • username-removed-636429 changed milestone to %9.1

    changed milestone to %9.1

    • Please register or sign in to reply