1/3 JS codestyle: camelcase

1. +++ b/core/misc/ajax.es6.js
   @@ -374,9 +374,9 @@
   -    this.element_settings = element_settings;
   +    this.elementSettings = elementSettings;
   

   ❓ I'm quite sure this is a BC break. Ajax commandos, modules overriding other parts of Drupal.Ajax etc. might all access this information, don't you think so?

2. +++ b/core/modules/color/color.es6.js
   @@ -91,10 +91,10 @@
   -       * @function Drupal.color~shift_color
   +       * @function Drupal.color~shiftColor
           *
           * @param {string} given
   
   @@ -106,7 +106,7 @@
   -      function shift_color(given, ref1, ref2) {
   +      function shiftColor(given, ref1, ref2) {
   

   Mh, so this is a public method, renaming that might be tricky as well. Here is an idea: a) still provide shift_color() which calls to shiftColor. This single line could then be ignored

3. +++ b/core/modules/views/js/ajax_view.es6.js
   @@ -49,12 +49,12 @@
        // Retrieve the path to use for views' ajax.
   -    let ajax_path = drupalSettings.views.ajax_path;
   +    let ajaxPath = drupalSettings.views.ajaxPath;
   

   🔧 If we change drupalSettings we need to change the PHP for that as well ... On top of that this might be a BC break. What about providing both ajax_path and ajaxPath and call it a day?

Based on #3, this should be NW

Updated patch for @dawehner's comments.

Generated from https://github.com/drupal/drupal/compare/8.5.x...justafish:2915784.patch

Interdiff:
https://github.com/justafish/drupal/commit/e2f8fb29d4670b52f3de4352857f5...
https://github.com/justafish/drupal/commit/e2f8fb29d4670b52f3de4352857f5...

@justafish
You forgot to run babel: yarn run build:js

Patch:
https://github.com/justafish/drupal/compare/8.5.x...justafish:2915784
https://github.com/justafish/drupal/compare/8.5.x...justafish:2915784.patch

Interdiff:
https://github.com/justafish/drupal/commit/e31ec2c6690d7bac9d16d24cb0216...
https://github.com/justafish/drupal/commit/e31ec2c6690d7bac9d16d24cb0216...

#8 looks good to me.

Ran yarn lint:core-js-passing and the only error was

/Users/ted.bowman/Sites/octo-www/d8_ux/core/modules/color/color.es6.js
157:16 error Identifier 'shift_color' is not in camel case camelcase

✖ 1 problem (1 error, 0 warnings)

This was expected because of problem in #3.3

I see that @justafish added
let ajaxPath = drupalSettings.views.ajax_path; // eslint-ignore-line camelcase
To ignore this problem I tried to do the same for shift_color but is not working for me.

UPDATE. @drpal point out don't need the // eslint-ignore-line camelcase because a key on specific object is not linted.

Added
function shift_color(given, ref1, ref2) { // eslint-disable-line camelcase
to ignore this. Also updated the docs on shift_color

👍

This sounds like a job for git diff --color-words=".". Winning.

Here are all the renamed names from the current patch (from a manual inspection):

element_settings
form_values
new_content_wrapped
new_content
source_id
tab_focus
tab_list
vertical_tab
filter_rows
region_message
region_items
shift_color
color_start
color_end
dependent_columns
field_id
link_title
ajax_path
self_settings
submit_buttons
display_id
base_element_settings
submit_buttons
old_value
gradient_start
gradient_end

I haven't checked the BC of all the things yet, but definitely glad to see us providing BC and a deprecation for element_settings. One thing that @drpal mentioned to me once before is that @deprecated isn't really a thing in JS. He recommended something else at the time and I've forgotten what it was. This is probably a good time to establish a couple best practices on how we do BC for JS, so setting NR to discuss whether there's something other than @deprecated we should be using here. Thanks!

I had a quick look of what react is doing and they totally use throw/console.warn, see https://github.com/facebook/react/blob/15-stable/src/shared/utils/deprec... and https://github.com/facebook/react/blob/15-stable/src/shared/utils/lowPri...

Okay so I was confused. It was actually a past conversation about @internal not being a thing in JS.

Also I am totally excited by #13! That is great.

So while I still think we should add this to our BC policy, what's in the patch is probably okay. :) Setting back to RTBC and looking a bit more closely at the current patch.

Can we adopt the same message format for the @deprecated that we use for PHP? Like:

@deprecated in Drupal 8.5.x and will be removed before 9.0.0. Use elementSettings() instead.

@see http://drupal.org/node/the-change-notice-nid

Edit: added the @see

Edit: Because that's four pieces of information the person needs, and so far this patch only includes one of them. :)

This issue would be adding our two very first deprecations in our JS code. :) So, I posted #2918868: [policy, no patch] Use a deprecation process for JavaScript similar to what we use for PHP code.

[ibnsina:drupal | Wed 19:06:56] $ grep -rFwf tmp * | grep ".es6.js"
core/misc/ajax.es6.js:    this.element_settings = elementSettings;
core/modules/color/color.es6.js:      function shift_color(given, ref1, ref2) { // eslint-disable-line camelcase
core/modules/views/js/ajax_view.es6.js:    let ajaxPath = drupalSettings.views.ajax_path;
core/modules/views/js/ajax_view.es6.js:    this.element_settings = {
core/modules/views/js/ajax_view.es6.js:    const selfSettings = $.extend({}, this.element_settings, {
core/modules/views/js/ajax_view.es6.js:      const selfSettings = $.extend({}, that.element_settings, {
core/modules/views/js/ajax_view.es6.js:    const selfSettings = $.extend({}, this.element_settings, {

(tmp is a file containing the list in #11.)

Lines 1-3 are the things we're providing BC for, but should the Views AJAX this.element_settings also have the same rename-and-deprecation that we are using elsewhere?

Also, for drupalSettings.views.ajax_path, should we rename and deprecate that too, or would that have (e.g.) notable performance cost?

Ah, I see that dawehner said:

If we change drupalSettings we need to change the PHP for that as well ... On top of that this might be a BC break. What about providing both ajax_path and ajaxPath and call it a day?

Can we file a followup for that (and anything else like that)? And also explicitly deprecate the old thing if possible.

Edit: also:

Ajax commandos

O.o

+++ b/core/misc/ajax.es6.js
@@ -388,9 +388,16 @@
-     * @type {Drupal.Ajax~element_settings}
+     * @deprecated See elementSettings.
+     *
+     * @type {Drupal.Ajax~elementSettings}
+     */
+    this.element_settings = elementSettings;
+

❓ I am wondering to which degree we could test this change? Could we have some code overriding the Ajax framework in a test and then check whether it is still working as expected?

On top of that I am wondering whether we could actually as of now remove entries from the .eslintrc.json and move it into ignore comments of the files which fail. This way other files could not introduce regressions and new code would have to comply with it already.

@drpal
Well, I'm wondering whether we could add more confidence into the patch by adding test. This would make the life of the committers much easier ...

Here is a patch which "proves" that both 'element_settings' and 'elementSettings' both work.

I'm stupid.

In general:

- The "after" of this patch looks SO much better on the eyes. Ahhh...
- Wow, +1 to the BC layers! That's a very nice touch. :)
- The AJAX test coverage is also great! But, is there any way that could be split out from this patch so we could look at that on its own and keep this patch to straight renames only? The tests look like useful additions on their own, regardless.

Found a few what look like missed spots in the review:

core/modules/views/js/ajax_view.es6.js, line 72:

     this.element_settings = {
+      url: ajaxPath + queryString,
     submit: settings,

top line still shows "element_settings". Same with lines 100, 117, 165.

In core/modules/system/tests/modules/js_ajax_test/js/js_ajax_test.ajax[.es6].js:

+    ajax.element_settings.cat = 'catbro';
+    debugger;

1) BZZZT on the debugger line, per Matt. ;)
2) We're assigning 'catbro' to element_settings still. This seems strange, since later on we have a test specifically for BC. I worry that in Drupal 9 when we blow away the @deprecated element_settings, we're going to create a test failure here.

On the test specifically:

+name: 'JS ajax test'
+type: module
+description: ''
+package: Testing
+version: VERSION
+core: 8.x

s/ajax/AJAX/g here and various other places (docblocks, etc.)

Rather than an empty string, let's give it an actual description that explains what's being tested.

+  public function submitForm(array &$form, FormStateInterface $form_state) {
+
+  }

I can't tell if this is a bug or by design that this function body is missing. If it's by design, let's leave a comment here to explain.

One thing I didn't do as part of my review: do a "global" grep in core's JS files for the old names for various variables (element_settings, form_values, source_id, etc.) to ensure there aren't any other stragglers. That probably would've caught the ajax_view problem above.

I just committed #2925064: [1/2] JS codestyle: no-restricted-syntax , so this one will need a messy re-roll. :\ Sorry about that!

1) BZZZT on the debugger line, per Matt. ;)

Dropped it, well, it proves at least that I did some manual testing.

2) We're assigning 'catbro' to element_settings still. This seems strange, since later on we have a test specifically for BC. I worry that in Drupal 9 when we blow away the @deprecated element_settings, we're going to create a test failure here.

Well, the point is: element_settings still works. We want to prove that old code continues to work.

s/ajax/AJAX/g here and various other places (docblocks, etc.)

I used Ajax, where possible now. Thank you!

I can't tell if this is a bug or by design that this function body is missing. If it's by design, let's leave a comment here to explain.

I added // Nothing to do. now.

Can we update the deprecations and add a change record as per #15? Thanks!

Oh, a more specific comment about why there's // Nothing to do. might be helpful also.

I don't think its worth documenting an empty submit method in a test at all.
"Implement an empty submit method because the form interface requires us to do" ....

Well, just that Nothing to do isn't really much better than an empty method. :) How about No submit handling for this form?

Oh and I agree with including the test coverage in the commit that adds the deprecations (inlcuding the old versions!) as @dawehner suggests. Massive +1 for having that since it makes it clear the patch is safe.

I also agree with Angie's point though. My default would (again) be to suggest separate issues, a first 1-2 which renames and deprecates the things where we would need to think about BC at least, with their tests, and then a big followup that renames everything else that is local variables and clearly doesn't need BC. That got pushback on the other issues so I'm reluctant to suggest it, but it's what I would suggest for a PHP patch as well. :)

Oh, as for Angie's question about introducing a test failure when we remove the deprecated code in D9, I think that's a good thing since it also reminds us that we can remove that part of the test. :) We haven't yet really started separating out legacy/deprecation tests for PHP so I don't think we need to try to do so here. It could be a followup or something.

Unassigned for now.

I also agree with Angie's point though. My default would (again) be to suggest separate issues, a first 1-2 which renames and deprecates the things where we would need to think about BC at least, with their tests, and then a big followup that renames everything else that is local variables and clearly doesn't need BC. That got pushback on the other issues so I'm reluctant to suggest it, but it's what I would suggest for a PHP patch as well. :)

I guess its fine to have 2 issues which are pure internal refactorings and then a third issue to actually reach the goal of fixing our eslintrc.

Well, just that Nothing to do isn't really much better than an empty method. :) How about No submit handling for this form?

Here is a list of form submit methods which do the same:

• \Drupal\form_test\FormTestAutocompleteForm
• \Drupal\form_test\Form\FormTestGroupVerticalTabsForm::submitForm
• \Drupal\form_test\Form\FormTestCheckboxTypeJugglingForm::submitForm
• \Drupal\form_test\Form\FormTestLabelForm::submitForm
• \Drupal\form_test\Form\FormTestRangeInvalidForm::submitForm
• \Drupal\form_test\Form\FormTestPlaceholderForm::submitForm
• ...

Looking at the patch there are two API changes:

• Rename ajax_settings to ajaxSettings
• Rename shift_color to shiftColor

So just these two changes + the test for ajax_settings should be in one issue?

Then another issue to rename all the uses of ajax_settings to ajaxSettings

And then another issue to rename all the other local variables to camelCase? At least for me this process is a bit sad, because along the process in 1 and 2 you never know whether you caught all instances. You just see that once in step 3 you are actually able to change the eslintrc file, but maybe its fine to accept that the current patch is fine as it is, it just needs to be split up

Works for me. Ideally we wouldn't remove the eslintrc line yet, but this is sadly the problem of splitting it up into these small packages.

"Unable to allocate memory for pool" seems a bit spurious...

Let's remove the eslint line for now :(

Yeah for phpcs rules when a rule's fixes are too big/varied to be a single scope, we do one of two things:

1. Enable the rule on the last child issue. (By far the most common because there might be several conceptual scopes that are found in many different places.)
2. Ignore specific lines/ranges/files temporarily until they're fixed (if it's a small number/easy to do).

So I think we can do the same here.

Thank you @matt!

Ok great, this is all nice and straight-forward.

I did raise an eyebrow at:

+++ b/core/modules/content_translation/content_translation.admin.es6.js
@@ -17,10 +17,11 @@
       const $context = $(context);
       const options = drupalSettings.contentTranslationDependentOptions;
       let $fields;
+      let dependentColumns;
 
-      function fieldsChangeHandler($fields, dependent_columns) {
+      function fieldsChangeHandler($fields, dependentColumns) {

...there's no corresponding "- let dependent_columns;" line to the "+ let dependentColumns;" line. @drpal explained that this was an optimization because the old code was re-instantiating the variable on every loop that he caught while doing the camel case conversions. That's a nice improvement, but in general it'd be great to split these types of things out into a separate "optimize JS code" issues (I'm not going to ask for it here, because it's just this one instance, and this poor issue has already been "could you please split this out into ..." to deathed. :P Just in general, here is some guidance on patch scoping: http://webchick.net/please-stop-eating-baby-kittens)

Everything else, however, is just straight-forward s/foo_bar/fooBar/g; changes.

Ran the patch on simplytest.me, manually checking:

- Machine names + vertical tabs on content type admin
- Dragging/dropping table rows in the block admin page and menu admin page.
- CKEditor (both end-user and config interface)
- States system used in Content Translation admin page
- Color module preview

No problems detected.

Committed and pushed to 8.5.x. w00t! Onward! :)

I had to revert this because a revert was requested on #2925064: [1/2] JS codestyle: no-restricted-syntax and it conflicted. :( @webchick and I have both reviewed this closely already so if we can just reroll this it can probably go back in. I've postponed the other on this since this one is easier to review.

First time attempt at a reroll - not a straight reroll as a couple of things in the content_translation.admin were different.

Thank you for the revert @xjm. Reverts are cheap :)

Thank you @mikejw for the quick reroll!

I'm guessing that was meant to be this. :)

Can't seem to commit this because https://github.com/alexpott/d8githooks is giving me:

$ node ./scripts/js/babel-es6-build.js --check --file /Users/angie.byron/Sites/8.x/core/modules/content_translation/content_translation.admin.es6.js
[09:40:31] '/Users/angie.byron/Sites/8.x/core/modules/content_translation/content_translation.admin.es6.js' is being checked.
[09:40:32] '/Users/angie.byron/Sites/8.x/core/modules/content_translation/content_translation.admin.es6.js' is not updated.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
yarn run v0.24.6
$ node ./scripts/js/babel-es6-build.js --check --file /Users/angie.byron/Sites/8.x/core/modules/content_translation/content_translation.admin.es6.js
[09:40:33] '/Users/angie.byron/Sites/8.x/core/modules/content_translation/content_translation.admin.es6.js' is being checked.
[09:40:34] '/Users/angie.byron/Sites/8.x/core/modules/content_translation/content_translation.admin.es6.js' is not updated.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
yarn run v0.24.6

Any ideas?

I don't get any issues - did you run a `git pull` on 8.5.x before you tried? I will now double check on fresh checkout of 8.5.x.

Ok, I found something :) yeah, needed to run build.

Thank you @mikejw

Okay, take two! :)

Committed and pushed to 8.5.x. Thanks!