Problem/Motivation

Provide a smooth, fast & safe way to transform our code to ES6.

Proposed resolution

Use https://github.com/facebook/jscodeshift as toolkit.

Remaining tasks

- Pick the high-quality codemod plugins and use in Drupal

User interface changes

- N/A

API changes

- N/A

Data model changes

- N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet created an issue. See original summary.

nod_’s picture

I have no opinion on which tool to use for making this happen. If this one is better, make a patch, i'll test and we'll probably change for this :)

nod_’s picture

Tried it out, looks like it'd be more useful for things like refactoring during version upgrades than use as a tool to compile our JS. I didn't see how we could create a new file from that.

nod_’s picture

Oh ok, my mistake. You're right we should use that.

nod_’s picture

Title: Evaluate and Use jscodeshift for ES3/5 to ES6 transform in CORE & Contributed modules » Use jscodeshift for ES3/5 to ES6 transform in CORE & Contributed modules
Priority: Normal » Major
FileSize
4.83 KB

This is awesome :) Worked on it today. Confusing to get started but after a while is mostly ok. Attached a codemod that replace calls to jquery each with for of loops (since they support it now).

After applying some codemods from https://github.com/cpojer/js-codemod (no-vars and arrow-function) and mine, the code looks like this (and running eslint --fix):

Before

  Drupal.behaviors.menuUiLinkAutomaticTitle = {
    attach: function (context) {
      var $context = $(context);
      $context.find('.menu-link-form').each(function () {
        var $this = $(this);
        // Try to find menu settings widget elements as well as a 'title' field
        // in the form, but play nicely with user permissions and form
        // alterations.
        const $checkbox = $this.find('.js-form-item-menu-enabled input');
        const $link_title = $context.find('.js-form-item-menu-title input');
        const $title = $this.closest('form').find('.js-form-item-title-0-value input');
        // Bail out if we do not have all required fields.
        if (!($checkbox.length && $link_title.length && $title.length)) {
          return;
        }
        // Whenever the value is changed manually, disable this behavior.
        $link_title.on('keyup', function () {
          $link_title.data('menuLinkAutomaticTitleOverridden', true);
        });
        // Global trigger on checkbox (do not fill-in a value when disabled).
        $checkbox.on('change', function () {
          if ($checkbox.is(':checked')) {

          //...
          $checkbox.closest('.vertical-tabs-pane').trigger('summaryUpdated');
          $checkbox.trigger('formUpdated');
        });
        // Take over any title change.
        $title.on('keyup', function () {
          if (/* … */) {
            //...
          }
        });
      });
    }
  };

After


  Drupal.behaviors.menuUiLinkAutomaticTitle = {
    attach: function (context) {
      const $context = $(context);
      for (let element of $context.find('.menu-link-form')) {
        const $this = $(element);
        // Try to find menu settings widget elements as well as a 'title' field
        // in the form, but play nicely with user permissions and form
        // alterations.
        const $checkbox = $this.find('.js-form-item-menu-enabled input');
        const $link_title = $context.find('.js-form-item-menu-title input');
        const $title = $this.closest('form').find('.js-form-item-title-0-value input');
        // Bail out if we do not have all required fields.
        if (!($checkbox.length && $link_title.length && $title.length)) {
          return;
        }
        // Whenever the value is changed manually, disable this behavior.
        $link_title.on('keyup', () => {
          $link_title.data('menuLinkAutomaticTitleOverridden', true);
        });
        // Global trigger on checkbox (do not fill-in a value when disabled).
        $checkbox.on('change', () => {
          if ($checkbox.is(':checked')) {

          //...
          $checkbox.closest('.vertical-tabs-pane').trigger('summaryUpdated');
          $checkbox.trigger('formUpdated');
        });
        // Take over any title change.
        $title.on('keyup', () => {
          if (/* … */) {
            //...
          }
        });
      }
    }
  };

It'll take a while to get all the codemods working but it's definitely worth it. Instant upgrade for any drupal js. the amount of modifications don't require a compilation to work on modern browsers (see https://kangax.github.io/compat-table/es6/).

nod_’s picture

And maybe the most visible change is in theme functions:

before

Drupal.theme.quickeditFormContainer = function (settings) {
  var html = '';
  html += '<div id="' + settings.id + '" class="quickedit-form-container">';
  html += '  <div class="quickedit-form">';
  html += '    <div class="placeholder">';
  html += settings.loadingMsg;
  html += '    </div>';
  html += '  </div>';
  html += '</div>';
  return html;
};

Drupal.theme.localeTranslateChangedWarning = function () {
  return '<div class="clearfix messages messages--warning">' + 
    Drupal.theme('localeTranslateChangedMarker') + ' ' + 
    Drupal.t('Changes made in this table will not be saved until the form is submitted.') + 
  '</div>';
};


after

Drupal.theme.quickeditFormContainer = function (settings) {
  return `<div id="${settings.id}" class="quickedit-form-container">
    <div class="quickedit-form">
      <div class="placeholder">
        ${settings.loadingMsg}
      </div>
    </div>
  </div>`;
};

Drupal.theme.localeTranslateChangedWarning = function () {
  return `<div class="clearfix messages messages--warning">
    ${Drupal.theme('localeTranslateChangedMarker')} 
    ${Drupal.t('Changes made in this table will not be saved until the form is submitted.')}
  </div>`;
};


nod_’s picture

Added some more changes:

before

$(context).find('[data-…]').once('block-highlight').each(function () {
  var $container = $(this);
}).remove();

$.extend(this, defaults, element_settings);

after

let $elements = $(context).find('[data-…]').once('block-highlight');
for (const element of $elements) {
  var $container = $(element);
}
$elements.remove();

Object.assign(this, defaults, element_settings);

Some issues with nested .each() in the first rule but it could be manually dealt with, there are about 4 places where it fails on core code.

If you're wondering about the utility of getting rid of .each() call, when #2402103: Add once.js to core lands, it'll be easier to refactor. Not like it's required but I'm happy when we don't use jquery-specific apis. It does help for passing from jquery to native selector, since this is possible

for (let element of document.querySelectorAll('[data-…]')) {

}

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Wim Leers’s picture