Trigger deprecation notifications on JavaScript deprecations to notify developers that deprecated code is being used

This looks like a great start! 😃

Points 1 and 2 review the key parts of what #3 proposes, the other points look at the impact of those points and the bigger picture.

1. +++ b/core/misc/drupal.es6.js
   @@ -541,6 +541,51 @@ window.Drupal = { behaviors: {}, locale: {} };
   +  Drupal.deprecationWarning = function(message) {
   

   A method for triggering a deprecation warning using the console.

   There is a suppression mechanism, which is turned on by default.

   👍 Seems reasonable.

   🤔 Primary question: why "deprecation warning" and not "deprecation error"? That's what it's called everywhere else.

2. +++ b/core/misc/drupal.es6.js
   @@ -541,6 +541,51 @@ window.Drupal = { behaviors: {}, locale: {} };
   +  Drupal.deprecateProperty = (target, deprecatedProp, message) => {
   

   This then is a helper to make it easy to declare deprecated properties, because they me be declared in places where that declaration cannot be followed by a function call (to Drupal.deprecationWarning()).

   👍 Makes sense.

   🤔 I think Drupal.deprecatedProperty() makes slightly more sense? What is your reasoning for this naming?

3. +++ b/core/misc/ajax.es6.js
   @@ -1118,20 +1122,24 @@
   -  Drupal.theme.ajaxWrapperNewContent = ($newContent, ajax, response) =>
   ...
   +  Drupal.theme.ajaxWrapperNewContent = ($newContent, ajax, response) => {
   

   🤔🤓 I'm wondering if we want a proxy-based approach just like you have for the property, because that would allow us to not have to reformat (indent) existing function definitions?

4. +++ b/core/misc/drupal.es6.js
   @@ -541,6 +541,51 @@ window.Drupal = { behaviors: {}, locale: {} };
   +   * @see https://www.drupal.org/core/deprecation
   

   🤔 IMHO this should link to https://www.drupal.org/core/deprecation#javascript

5. +++ b/core/modules/system/system.module
   @@ -788,6 +788,7 @@ function system_js_settings_alter(&$settings, AttachedAssetsInterface $assets) {
   +  $settings['suppressDeprecationWarnings'] = TRUE;
   

   🤔 It's very important here to turn on deprecation warnings (errors?) when it makes sense.

   I'm sure that's something for a future patch iteration.

In my opinion, it's better to directly call the function to trigger a warning whenever possible since it results in simpler code

Yep, that's fair. I only thought of it because there is a helper method for properties, so we might want one for functions too. Then again, for properties it's necessary (for the reasons I provided), for functions it's not.

Should it be something that a module could override or should there be a configuration in core to turn off the suppression?

Yeah, good question. I'm inclined to say "if PHP assertions are turned on, you want this to be turned on too". But that's not necessarily true: PHP assertions matter to back end developers, JS deprecations matter to front end developers, you only want both if you're working on both ends simultaneously.

I'm afraid this goes back to #1537198: Add a Production/Development Toggle To Core, which has been an unsolved problem for more years than I'd like to know 🙃

These methods for logging warnings in the console seem like an ideal approach. I did some research into how React displays warnings in strict mode, and how other js libraries handle deprecations. Logging to the console is standard since there isn't a native feature of the language. Typically with a React app where there is a compile step the "production" build won't display these kind of developer notices. That's not an option for Drupal though since the compiled files are the ones that get loaded and used.

I'm inclined to say "if PHP assertions are turned on, you want this to be turned on too". But that's not necessarily true: PHP assertions matter to back end developers, JS deprecations matter to front end developers, you only want both if you're working on both ends simultaneously.

I agree that php assertions and js deprecations shouldn't be connected, but what about something more user friendly like the "Logging and Errors" config? I think it's system.logging.error_level.

+++ b/core/modules/system/system.module
@@ -788,6 +788,7 @@ function system_js_settings_alter(&$settings, AttachedAssetsInterface $assets) {
+  $settings['suppressDeprecationErrors'] = TRUE;

This could just be TRUE only for error_level: hide so that the warnings would always appear unless deliberately suppressed.
We want developers to notice these warnings, but to be easily suppressed by setting this option to "none" on production.
I think most developers or site builders would think to go here (admin/config/development/logging) first if they wanted to hide those console warnings.

+++ b/core/misc/drupal.js
@@ -173,6 +173,23 @@ window.Drupal = { behaviors: {}, locale: {} };
+  Drupal.deprecatedProperty = function (target, deprecatedProp, message) {
+    return new Proxy(target, {

I was curious to see how the compiler handled this, but it is still using Proxy. I don't think Proxy is supported by IE 11 since it is an ES6 feature of javascript.

Likewise:

+++ b/core/misc/drupal.js
@@ -173,6 +173,23 @@ window.Drupal = { behaviors: {}, locale: {} };
+        return Reflect.get.apply(Reflect, arguments);

Reflect is also not supported in IE11.

This is cool 👍

Is there a reason deprecationError is a regular function() and deprecatedProperty is an arrow function?

You could also use console.trace instead of console.warn, so it's easier to find where the message was triggered and doesn't mean you need to rely so much on the error message text.

I prefer to pass objects through to functions in JavaScript, it's much more flexible if you want to expand later. I'd also set default values where possible.

Drupal.deprecationError = function({ message: "Function deprecated" }) {
  if (
     drupalSettings.suppressDeprecationErrors === false &&
     typeof console !== 'undefined' &&
     console.trace
   ) {
     console.trace(`[Deprecation] ${message}`);
   }
};

Drupal.deprecatedProperty = ({ target, deprecatedProp, message = "Deprecated property" }) => {
  return new Proxy(target, {
    get(target, key) {
      if (key === deprecatedProp) {
        Drupal.deprecationError(message);
      }
      return Reflect.get(...arguments);
    },
  });
};

Looks good (assuming the tests pass)! 💃

Yay! Thanks, @justafish! :)

I agree that the JS side of things here looks ready, but I think we should consider implementing @zrpnr's suggestion too:

I'm inclined to say "if PHP assertions are turned on, you want this to be turned on too". But that's not necessarily true: PHP assertions matter to back end developers, JS deprecations matter to front end developers, you only want both if you're working on both ends simultaneously.

I agree that php assertions and js deprecations shouldn't be connected, but what about something more user friendly like the "Logging and Errors" config? I think it's system.logging.error_level.

+++ b/core/modules/system/system.module
@@ -788,6 +788,7 @@ function system_js_settings_alter(&$settings, AttachedAssetsInterface $assets) {
+  $settings['suppressDeprecationErrors'] = TRUE;

This could just be TRUE only for error_level: hide so that the warnings would always appear unless deliberately suppressed.
We want developers to notice these warnings, but to be easily suppressed by setting this option to "none" on production.
I think most developers or site builders would think to go here (admin/config/development/logging) first if they wanted to hide those console warnings.

Or if we don’t, at least open a follow-up for it.

So we still need a follow-up to determine a smarter default for suppressDeprecationErrors. But since this issue is at least major (or is it critical?), we should go ahead and get this committed ASAP.

I disabled this when Proxy or Reflect isn't supported which seems fine to me since this is a developer-facing feature so it might be safe to assume that they're not running IE 11.

That's a good point, very unlikely developers would be working using IE11, but the additional check you added is an improvement and an additional safeguard if warnings were accidentally left enabled on production.

+1 for passing objects to the functions, and using arrow functions everywhere!

Should we remove the @todo Add deprecation warning after it is possible from the docblock above Drupal.theme.ajaxWrapperNewContent and Drupal.theme.ajaxWrapperMultipleRootElements as part of this patch?

we still need a follow-up to determine a smarter default for suppressDeprecationErrors. But since this issue is at least major (or is it critical?), we should go ahead and get this committed ASAP.

Created #3082404: Follow-up to #3070521 Provide option to enable javascript deprecation warnings
I agree that this feature should not block this issue.

This is still RTBC since the latest patch was a small change and tests pass, nice work @lauriii !

Moved setting the suppressDeprecationErrors default value to core.libraries.yml to […]

👍 WHY DID I NOT THINK OF THIS!?

But … it's not in the patch?

Perfect.

I think I even worked on adding the default drupalSettings to core.libraries.yml way back when … yep: #2382557: Change JS settings into a separate asset type. This looks perfect now.

I wonder if it would be best to merge the two change records for this? I think the information on how to suppress the messages could be added at the end of https://www.drupal.org/node/3082634.

For #24

I still need my release note. :)

Probably we should tick all the boxes here since this significantly blah blah blah. :) Lauri has already signed off so I didn't add that tag.

I'm happy with this - I reviewed it back in #25 and only had questions about the change records. But I realise I didn't actually say 'this looks good' or anything like that.

I chatted with lauri and he suggested we needed a way to get deprecation messages in our FunctionJavascript phpunit tests if deprecated JS functions are called.

Here is a try at that.

This starts from some work lauri had done after #22

1. +++ b/core/modules/system/tests/modules/js_deprecation_log_test/js/js_deprecation_log.es6.js
   @@ -0,0 +1,21 @@
   +      localStorage.setItem(
   +        'js_deprecation_log_test.warnings',
   +        JSON.stringify(warnings),
   +      );
   

   We store the warning in localstorage because there may be calls on previous pages and we need to be able evaluate at the end of the test if any JS deprecated calls were made.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptDeprecationTest.php
   @@ -0,0 +1,25 @@
   +  /**
   +   * @expectedDeprecation Javascript Deprecation: Here be llamas.
   +   */
   +  public function testJavascriptDeprecation() {
   

   Here we test for a know deprecation message

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php
   @@ -17,6 +17,11 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static $modules = ['js_deprecation_log_test'];
   

   We have to always have this test module enabled because it intercepts the calls to console.warn() to ensure that for all calls across across all page loads in test we will detect deprecated JS calls.

4. +++ b/core/tests/Drupal/Nightwatch/Assertions/consoleMessageExists.js
   @@ -0,0 +1,19 @@
   diff --git a/core/tests/Drupal/Nightwatch/Tests/jsDeprecationTest.js b/core/tests/Drupal/Nightwatch/Tests/jsDeprecationTest.js
   
   diff --git a/core/tests/Drupal/Nightwatch/Tests/jsDeprecationTest.js b/core/tests/Drupal/Nightwatch/Tests/jsDeprecationTest.js
   new file mode 100644
   
   new file mode 100644
   index 0000000000..2f50de2131
   
   index 0000000000..2f50de2131
   --- /dev/null
   
   --- /dev/null
   +++ b/core/tests/Drupal/Nightwatch/Tests/jsDeprecationTest.js
   
   +++ b/core/tests/Drupal/Nightwatch/Tests/jsDeprecationTest.js
   +++ b/core/tests/Drupal/Nightwatch/Tests/jsDeprecationTest.js
   @@ -0,0 +1,26 @@
   

   @lauriii made all the changes to the nightwatch stuff so I will let him explain those.

5. If are calling other deprecated JS code in other test then a lot of other tests should fail.

+++ a/core/package.json
@@ -26,7 +26,7 @@
+    "chromedriver": "^75.1.0",
-    "chromedriver": "^77.0.0",

+++ a/core/yarn.lock
@@ -1233,10 +1233,10 @@
+chromedriver@^75.1.0:
+  version "75.1.0"
...
-chromedriver@^77.0.0:
-  version "77.0.0"

✅ 👎Why these changes? this was in #35, but fixed in #36 :)

Let's move deprecating actual production code to follow-up to make sure we have at least this in prior to 8.8.0-alpha1.

I think this is the right call. 👍

1. +++ b/core/modules/system/tests/modules/js_deprecation_test/js/js_deprecation_test.es6.js
   @@ -0,0 +1,22 @@
   +  const deprecatedFunction = () => {
   +    deprecationError({ message: 'This function is deprecated for testing purposes.' });
   +  };
   

   ✅ Sample deprecated JS function.

2. +++ b/core/modules/system/tests/modules/js_deprecation_test/js/js_deprecation_test.es6.js
   @@ -0,0 +1,22 @@
   +  const objectWithDeprecatedProperty = deprecatedProperty({
   

   ✅ Sample deprecated JS property.

3. +++ b/core/modules/system/tests/modules/js_deprecation_test/js/js_deprecation_test.es6.js
   @@ -0,0 +1,22 @@
   +  behaviors.testDeprecations = {
   +    attach: () => {
   +      deprecatedFunction();
   +      const deprecatedProperty =
   +        objectWithDeprecatedProperty.deprecatedProperty;
   +    },
   +  };
   +})(Drupal);
   

   ✅ Using both deprecated pieces of code. This triggers the deprecation errors.

4. +++ b/core/modules/system/tests/modules/js_deprecation_test/js_deprecation_test.module
   @@ -0,0 +1,13 @@
   +  $settings['suppressDeprecationErrors'] = FALSE;
   

   ✅ This allows the JS deprecation errors to actually show up.

5. +++ b/core/profiles/testing/testing.info.yml
   @@ -9,3 +9,4 @@ install:
   +  - js_deprecation_log_test
   

   🤔 Ohhhh! I did not expect this. But I think it might make sense. 👍 We need JS deprecation errors to be surfaced in all our functional tests, just like PHP deprecation errors.

   Worth nothing: this means that this won't work in other install profiles, like standard, demo_umami, or Drupal distributions like Thunder or Lightning. Do we believe that's okay?

   Shouldn't we move this to \Drupal\FunctionalJavascriptTests\WebDriverTestBase instead?

   Why is that not possible?

6. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptDeprecationTest.php
   @@ -0,0 +1,26 @@
   +  public static $modules = ['js_deprecation_test'];
   

   🤓 Übernit: Missing doc.

7. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptDeprecationTest.php
   @@ -0,0 +1,26 @@
   +   * @expectedDeprecation Javascript Deprecation: This function is deprecated for testing purposes.
   +   * @expectedDeprecation Javascript Deprecation: This property is deprecated for testing purposes.
   

   ✅ This proves deprecations are surfaced in tests 🥳

8. +++ b/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php
   @@ -108,6 +108,13 @@ protected function tearDown() {
   +      $warnings = $this->getSession()->evaluateScript("JSON.parse(localStorage.getItem('js_deprecation_log_test.warnings') || JSON.stringify([]))");
   +      foreach ($warnings as $warning) {
   +        if (strpos($warning, '[Deprecation]') === 0) {
   +          @trigger_error('Javascript Deprecation:' . substr($warning, 13), E_USER_DEPRECATED);
   +        }
   +      }
   

   ✅ This maps JS deprecation errors to PHP deprecation errors, so our existing testing infrastructure can surface it! 👏

9. +++ b/core/tests/Drupal/Nightwatch/Assertions/deprecationErrorExists.js
   --- /dev/null
   +++ b/core/tests/Drupal/Nightwatch/Assertions/noDeprecationErrors.js
   
   +++ b/core/tests/Drupal/Nightwatch/Tests/loginTest.js
   @@ -18,6 +18,7 @@ module.exports = {
   +      .assert.noDeprecationErrors();
   

   ✅ And this allows nightwatch JS tests to ensure there are no deprecation errors either.

I would have RTBC'd this, but I think point 5 needs more discussion still.

Maybe we should install it in both, \Drupal\FunctionalJavascriptTests\WebDriverTestBase and testing installation profile.

+1

So we need it in testing to make it work for Nightwatch tests. We need it in WebDriverTestBase so that it works in all functional JS tests.

Arguably, we shouldn't add it to testing, and instead add a nightwatch_testing install profile instead. Because it's possible and even likely that there are more customizations necessary for Nightwatch tests. Furthermore, arguably it does not make sense to have the page_cache and dynamic_page_cache modules installed for Nightwatch tests, since Nightwatch tests are solely about client side testing, not about server-side responses and their cacheability.

Together, that means I'm implicitly proposing nightwatch_testing.info.yml to look like this:

name: Nightwatch Testing
type: profile
description: 'Minimal profile for running Nightwatch tests. Includes absolutely required modules only.'
version: VERSION
core: 8.x
hidden: true
install:
  - js_deprecation_log_test

1. +++ b/core/profiles/nightwatch_testing/nightwatch_testing.info.yml
   @@ -0,0 +1,8 @@
   +name: Nightwatch Testing
   ...
   +  - js_deprecation_log_test
   
   +++ b/core/tests/Drupal/Nightwatch/Commands/drupalInstall.js
   @@ -25,7 +25,7 @@ exports.command = function drupalInstall({ setupFile = '' } = {}, callback) {
   -        `php ./scripts/test-site.php install ${setupFile} --base-url ${process.env.DRUPAL_TEST_BASE_URL} ${dbOption} --json`,
   +        `php ./scripts/test-site.php install ${setupFile} --install-profile nightwatch_testing --base-url ${process.env.DRUPAL_TEST_BASE_URL} ${dbOption} --json`,
   

   🥳 This ensures that Nightwatch tests install Drupal with this new nightwatch_testing install profile. 👍

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptDeprecationTest.php
   @@ -0,0 +1,26 @@
   +  public static $modules = ['js_deprecation_test'];
   

   🤓 Still the same nit as in #40.6.

3. +++ b/core/tests/Drupal/Nightwatch/Tests/jsDeprecationTest.js
   @@ -0,0 +1,28 @@
   +    browser.drupalInstall().drupalLoginAsAdmin(() => {
   +      browser
   +        .drupalRelativeURL('/admin/modules')
   +        .setValue('input[type="search"]', 'JS Deprecation test')
   +        .waitForElementVisible(
   +          'input[name="modules[js_deprecation_test][enable]"]',
   +          1000,
   +        )
   +        .click('input[name="modules[js_deprecation_test][enable]"]')
   +        .click('input[type="submit"]'); // Submit module form.
   +    });
   

   This is no longer necessary AFAICT? 😊 Ah no, the install profile installs js_deprecation_log_test (which results in the logging), this is installing js_deprecation_test which intentionally triggers deprecation errors so we can test it. 👍

4. +++ b/core/tests/Drupal/Nightwatch/Tests/loginTest.js
   @@ -17,7 +17,8 @@ module.exports = {
   +      .waitForElementVisible('body', 1000)
   

   🤔 Why do we need to add this wait now? This seems like a debug leftover? Oh … this is to allow the JS deprecation errors to have been triggered prior to us asserting there are no JS deprecation errors. 👍

This needed a follow-up for #39. Done: #3085926: Deprecate Drupal.theme.ajaxWrapperNewContent + Drupal.theme.ajaxWrapperMultipleRootElements.

Issue summary updated:

1. Consensus documented. Credited @justafish.
2. API additions documented.
3. Release note added.

Credited @zrpnr for his review in helping validate the approach. Also credited @tedbow for his crucial help in writing test coverage for this.

Change record added: https://www.drupal.org/node/3085933

EDIT: d'oh that already existed at https://www.drupal.org/node/3082634. Merged my text into @lauriii's. Keeping it around for now, to retain the history. Once Lauri's is published, mine can be deleted. 🤓

Updated documentation at https://www.drupal.org/core/deprecation#javascript: https://www.drupal.org/node/2856615/revisions/view/11570944/11590225

#46 failed because the tests switch to stark from classy and there was 1 css selector that didn't exist anymore.

Fixed

✅ https://www.drupal.org/node/3085950

+++ b/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php
@@ -108,6 +109,13 @@ protected function tearDown() {
+      $warnings = $this->getSession()->evaluateScript("JSON.parse(localStorage.getItem('js_deprecation_log_test.warnings') || JSON.stringify([]))");
+      foreach ($warnings as $warning) {
+        if (strpos($warning, '[Deprecation]') === 0) {
+          @trigger_error('Javascript Deprecation:' . substr($warning, 13), E_USER_DEPRECATED);
+        }

ChromeDriver has a very long-lived cache (in my experience), should we be removing the items from localStorage after each test to prevent pollution of subsequent tests?

should we be removing the items from localStorage after each test to prevent pollution of subsequent tests

Oh, that's a very interesting remark!

Can't we just use sessionStorage instead? That is cleared for every new tab and window by definition :)

1. Changing to use Session storage

2. +++ b/core/modules/system/tests/modules/js_deprecation_log_test/js/js_deprecation_log.es6.js
   @@ -0,0 +1,21 @@
   +      let warnings = JSON.parse(
   

   this should be const

3. +++ b/core/modules/system/tests/modules/js_deprecation_test/js/js_deprecation_test.es6.js
   @@ -0,0 +1,22 @@
   +    deprecationError({ message: 'This function is deprecated for testing purposes.' });
   

   This line is update when I run yarn prettier. Maybe I didn't run it on last patch?

   Ran prettier.

4. +++ b/core/tests/Drupal/Nightwatch/Assertions/deprecationErrorExists.js
   @@ -0,0 +1,22 @@
   +    return this.api.execute(function() {
   

   This be a arrow function

5. +++ b/core/tests/Drupal/Nightwatch/Assertions/noDeprecationErrors.js
   @@ -0,0 +1,22 @@
   +    return this.api.execute(function() {
   

   This should be an arrow function

my #57.3 & 4 changes broke the tests. I will defer @lauriii on this and assume his version was actually correct.

Hurray! :)

+1 RTBC

Can't really do a meaningful review of the js here as such, but the change record looks good to me, and fine with the nightwatch switching to stark change.

Explicitly tagging with 'needs documentation updates' which should happen asap once this is committed (or could probably happen just before too).

The pre-commit hooks complain about this patch

$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Volumes/files/code/git/core/drupal/core/misc/drupal.es6.js
[08:39:16] '/Volumes/files/code/git/core/drupal/core/misc/drupal.es6.js' is being checked.
✨  Done in 1.05s.

/Volumes/files/code/git/core/drupal/core/misc/drupal.es6.js
  592:37  error  Expected property shorthand  object-shorthand

✖ 1 problem (1 error, 0 warnings)
  1 error, 0 warnings potentially fixable with the `--fix` option.

$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Volumes/files/code/git/core/drupal/core/misc/drupal.es6.js
[08:39:19] '/Volumes/files/code/git/core/drupal/core/misc/drupal.es6.js' is being checked.
✨  Done in 0.57s.

/Volumes/files/code/git/core/drupal/core/misc/drupal.es6.js
  592:37  error  Expected property shorthand  object-shorthand

✖ 1 problem (1 error, 0 warnings)
  1 error, 0 warnings potentially fixable with the `--fix` option.

$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Volumes/files/code/git/core/drupal/core/modules/system/tests/modules/js_deprecation_log_test/js/js_deprecation_log.es6.js
[08:39:21] '/Volumes/files/code/git/core/drupal/core/modules/system/tests/modules/js_deprecation_log_test/js/js_deprecation_log.es6.js' is being checked.
✨  Done in 0.49s.

$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Volumes/files/code/git/core/drupal/core/modules/system/tests/modules/js_deprecation_log_test/js/js_deprecation_log.es6.js
[08:39:22] '/Volumes/files/code/git/core/drupal/core/modules/system/tests/modules/js_deprecation_log_test/js/js_deprecation_log.es6.js' is being checked.
✨  Done in 0.52s.

$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Volumes/files/code/git/core/drupal/core/modules/system/tests/modules/js_deprecation_test/js/js_deprecation_test.es6.js
[08:39:24] '/Volumes/files/code/git/core/drupal/core/modules/system/tests/modules/js_deprecation_test/js/js_deprecation_test.es6.js' is being checked.
[08:39:24] '/Volumes/files/code/git/core/drupal/core/modules/system/tests/modules/js_deprecation_test/js/js_deprecation_test.es6.js' is not updated.
error Command failed with exit code 1.

$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Volumes/files/code/git/core/drupal/core/modules/system/tests/modules/js_deprecation_test/js/js_deprecation_test.es6.js
[08:39:26] '/Volumes/files/code/git/core/drupal/core/modules/system/tests/modules/js_deprecation_test/js/js_deprecation_test.es6.js' is being checked.
[08:39:26] '/Volumes/files/code/git/core/drupal/core/modules/system/tests/modules/js_deprecation_test/js/js_deprecation_test.es6.js' is not updated.
error Command failed with exit code 1.

/Volumes/files/code/git/core/drupal/core/tests/Drupal/Nightwatch/Assertions/deprecationErrorExists.js
   4:38  error  Unexpected block statement surrounding arrow body; move the returned value immediately after the `=>`  arrow-body-style
  11:51  error  Unexpected block statement surrounding arrow body; move the returned value immediately after the `=>`  arrow-body-style
  16:47  error  Unexpected block statement surrounding arrow body; move the returned value immediately after the `=>`  arrow-body-style
  20:30  error  Unexpected block statement surrounding arrow body; move the returned value immediately after the `=>`  arrow-body-style
  21:29  error  Unexpected function expression                                                                         prefer-arrow-callback

✖ 5 problems (5 errors, 0 warnings)
  5 errors, 0 warnings potentially fixable with the `--fix` option.


/Volumes/files/code/git/core/drupal/core/tests/Drupal/Nightwatch/Assertions/noDeprecationErrors.js
   4:38  error  Unexpected block statement surrounding arrow body; move the returned value immediately after the `=>`  arrow-body-style
  11:51  error  Unexpected block statement surrounding arrow body; move the returned value immediately after the `=>`  arrow-body-style
  16:47  error  Unexpected block statement surrounding arrow body; move the returned value immediately after the `=>`  arrow-body-style
  20:30  error  Unexpected block statement surrounding arrow body; move the returned value immediately after the `=>`  arrow-body-style
  21:29  error  Unexpected function expression                                                                         prefer-arrow-callback

✖ 5 problems (5 errors, 0 warnings)
  5 errors, 0 warnings potentially fixable with the `--fix` option.

@larowlan sorry about that.

I did

yarn lint:core-js-passing --fix (this fix a bunch of files I discarded the ones not changed in this patch)
yarn build:js
yarn prettier

prettier didn't make any changes

Hope this passes

14 times

File ignored because of a matching ignore pattern. Use "--no-ignore" to override.

caused this test to be marked as Build successful despite all tests passing. This is a pre-existing and known issue with DrupalCI + eslint tooling.

If I try to commit #63 locally with d8githooks, it does pass all tests:

$ git ci -am 63
Installing or updating dependencies...
> Drupal\Core\Composer\Composer::ensureComposerVersion
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update
Package phpunit/phpunit-mock-objects is abandoned, you should avoid using it. No replacement was suggested.
Generating autoload files
> Drupal\Core\Composer\Composer::preAutoloadDump
> Drupal\Core\Composer\Composer::ensureHtaccess
    Skipped installation of bin bin/composer for package composer/composer: file not found in package
yarn install v1.17.3
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
success Already up-to-date.
✨  Done in 0.33s.


Checking changed files...
PHPCS: core/core.libraries.yml passed
yarn run v1.17.3
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/wim.leers/Work/d8/core/misc/drupal.es6.js
[10:10:53] '/Users/wim.leers/Work/d8/core/misc/drupal.es6.js' is being checked.
✨  Done in 0.97s.
yarn run v1.17.3
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/wim.leers/Work/d8/core/misc/drupal.es6.js
[10:10:55] '/Users/wim.leers/Work/d8/core/misc/drupal.es6.js' is being checked.
✨  Done in 0.41s.
yarn run v1.17.3
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/wim.leers/Work/d8/core/modules/system/tests/modules/js_deprecation_log_test/js/js_deprecation_log.es6.js
[10:10:56] '/Users/wim.leers/Work/d8/core/modules/system/tests/modules/js_deprecation_log_test/js/js_deprecation_log.es6.js' is being checked.
✨  Done in 0.33s.
yarn run v1.17.3
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/wim.leers/Work/d8/core/modules/system/tests/modules/js_deprecation_log_test/js/js_deprecation_log.es6.js
[10:10:58] '/Users/wim.leers/Work/d8/core/modules/system/tests/modules/js_deprecation_log_test/js/js_deprecation_log.es6.js' is being checked.
✨  Done in 0.35s.
PHPCS: core/modules/system/tests/modules/js_deprecation_log_test/js_deprecation_log_test.info.yml passed
PHPCS: core/modules/system/tests/modules/js_deprecation_log_test/js_deprecation_log_test.libraries.yml passed
PHPCS: core/modules/system/tests/modules/js_deprecation_log_test/js_deprecation_log_test.module passed
yarn run v1.17.3
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/wim.leers/Work/d8/core/modules/system/tests/modules/js_deprecation_test/js/js_deprecation_test.es6.js
[10:10:59] '/Users/wim.leers/Work/d8/core/modules/system/tests/modules/js_deprecation_test/js/js_deprecation_test.es6.js' is being checked.
✨  Done in 0.35s.
yarn run v1.17.3
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/wim.leers/Work/d8/core/modules/system/tests/modules/js_deprecation_test/js/js_deprecation_test.es6.js
[10:11:00] '/Users/wim.leers/Work/d8/core/modules/system/tests/modules/js_deprecation_test/js/js_deprecation_test.es6.js' is being checked.
✨  Done in 0.36s.
PHPCS: core/modules/system/tests/modules/js_deprecation_test/js_deprecation_test.info.yml passed
PHPCS: core/modules/system/tests/modules/js_deprecation_test/js_deprecation_test.libraries.yml passed
PHPCS: core/modules/system/tests/modules/js_deprecation_test/js_deprecation_test.module passed
PHPCS: core/modules/system/tests/modules/js_deprecation_test/js_deprecation_test.routing.yml passed
PHPCS: core/modules/system/tests/modules/js_deprecation_test/src/Controller/JsDeprecationTestController.php passed
PHPCS: core/profiles/nightwatch_testing/config/optional/locale.settings.yml passed
PHPCS: core/profiles/nightwatch_testing/nightwatch_testing.info.yml passed
PHPCS: core/tests/Drupal/FunctionalJavascriptTests/JavascriptDeprecationTest.php passed
PHPCS: core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php passed
Committed <a href="https://git.drupalcode.org/project/drupal/commit/844e295">844e295</a> and pushed to 8.8.x. Thanks!

*** Does it need a change record? ***
*** Should it be in the release notes? ***
[8.8.x 844e295269] 63
 27 files changed, 422 insertions(+), 12 deletions(-)
 create mode 100644 core/modules/system/tests/modules/js_deprecation_log_test/js/js_deprecation_log.es6.js
 create mode 100644 core/modules/system/tests/modules/js_deprecation_log_test/js/js_deprecation_log.js
 create mode 100644 core/modules/system/tests/modules/js_deprecation_log_test/js_deprecation_log_test.info.yml
 create mode 100644 core/modules/system/tests/modules/js_deprecation_log_test/js_deprecation_log_test.libraries.yml
 create mode 100644 core/modules/system/tests/modules/js_deprecation_log_test/js_deprecation_log_test.module
 create mode 100644 core/modules/system/tests/modules/js_deprecation_test/js/js_deprecation_test.es6.js
 create mode 100644 core/modules/system/tests/modules/js_deprecation_test/js/js_deprecation_test.js
 create mode 100644 core/modules/system/tests/modules/js_deprecation_test/js_deprecation_test.info.yml
 create mode 100644 core/modules/system/tests/modules/js_deprecation_test/js_deprecation_test.libraries.yml
 create mode 100644 core/modules/system/tests/modules/js_deprecation_test/js_deprecation_test.module
 create mode 100644 core/modules/system/tests/modules/js_deprecation_test/js_deprecation_test.routing.yml
 create mode 100644 core/modules/system/tests/modules/js_deprecation_test/src/Controller/JsDeprecationTestController.php
 create mode 100644 core/profiles/nightwatch_testing/config/optional/locale.settings.yml
 create mode 100644 core/profiles/nightwatch_testing/nightwatch_testing.info.yml
 create mode 100644 core/tests/Drupal/FunctionalJavascriptTests/JavascriptDeprecationTest.php
 create mode 100644 core/tests/Drupal/Nightwatch/Assertions/deprecationErrorExists.js
 create mode 100644 core/tests/Drupal/Nightwatch/Assertions/noDeprecationErrors.js
 create mode 100644 core/tests/Drupal/Nightwatch/Tests/jsDeprecationTest.js

00:59:31.733    SyntaxError: Error while running "noDeprecationErrors" command: Unexpected token o in JSON at position 1
00:59:31.733        at JSON.parse (<anonymous>)
00:59:31.733        at AssertionInstance.value.result [as value] (/var/www/html/core/tests/Drupal/Nightwatch/Assertions/noDeprecationErrors.js:6:40)
00:59:31.733        at <anonymous>
00:59:31.733        at process._tickCallback (internal/process/next_tick.js:189:7)
00:59:31.734 
00:59:31.734 FAILED: 1 errors and  2 passed (135ms)
00:59:31.734    SyntaxError: Error while running "noDeprecationErrors" command: Unexpected token o in JSON at position 1
00:59:31.734        at JSON.parse (<anonymous>)
00:59:31.734        at AssertionInstance.value.result [as value] (/var/www/html/core/tests/Drupal/Nightwatch/Assertions/noDeprecationErrors.js:6:40)
00:59:31.734        at <anonymous>
00:59:31.734        at process._tickCallback (internal/process/next_tick.js:189:7)
00:59:31.878 
00:59:31.878 [Js Deprecation Test] Test Suite
00:59:31.878 ================================
00:59:37.132 ✔ Element <input[name="modules[js_deprecation_test][enable]"]> was visible after 540 milliseconds.
00:59:38.493 Running:  Test JavaScript deprecations
00:59:38.493 
00:59:38.593 ✔ Element <body> was visible after 27 milliseconds.
00:59:38.624 ✔ Testing if element <h1> contains text: "JsDeprecationTest"  - 31 ms.
00:59:38.638 
00:59:38.638 OK. 2 assertions passed. (145ms)
00:59:38.983 
00:59:38.983 [Login Test] Test Suite
00:59:38.983 =======================
00:59:42.270 Running:  Test login
00:59:42.270 
00:59:43.697 ✔ Expected element <.user-role-form .machine-name-value> to be visible in 2000ms - condition was met in 538ms
00:59:45.614 ✔ User "user" was created successfully  - 36 ms.
00:59:46.058 ✔ Passed [equal]: The user "user" was logged in.
00:59:46.146 ✔ Element <body> was visible after 26 milliseconds.
00:59:46.181 ✔ Testing if element <h1> contains text: "Reports"  - 35 ms.
00:59:46.187 
00:59:46.187 OK. 5 assertions passed. (3.917s)
00:59:46.482 
00:59:46.482 [States Test] Test Suite
00:59:46.482 ========================
00:59:51.745 ✔ Element <input[name="modules[form_test][enable]"]> was visible after 544 milliseconds.
00:59:54.033 Running:  Test form with state API
00:59:54.033 
00:59:54.132 ✔ Element <body> was visible after 26 milliseconds.
00:59:54.157 ✔ Element <input[name="textfield"]> was not visible after 25 milliseconds.
00:59:54.164 
00:59:54.164 OK. 2 assertions passed. (132ms)
00:59:54.491 _________________________________________________
00:59:54.491 
00:59:54.491 TEST FAILURE: 1 error during execution 0 assertions failed, 15 passed. 26.871s
00:59:54.491 
00:59:54.491  ✖ exampleTest
00:59:54.491  – Page objects test page (135ms)
00:59:54.491 
00:59:54.491   SyntaxError: Error while running "noDeprecationErrors" command: Unexpected token o in JSON at position 1
00:59:54.491        at JSON.parse (<anonymous>)
00:59:54.491        at AssertionInstance.value.result [as value] (/var/www/html/core/tests/Drupal/Nightwatch/Assertions/noDeprecationErrors.js:6:40)
00:59:54.491        at <anonymous>
00:59:54.491        at process._tickCallback (internal/process/next_tick.js:189:7)

So nightwatch tests are broken. Not the nicest reporting from DrupalCI :(

Sorry all. Was having some issue with linting breaking the tests.

+++ b/core/tests/Drupal/Nightwatch/Assertions/noDeprecationErrors.js
@@ -0,0 +1,23 @@
+  this.command = callback =>
+    this.api.execute(
+      () => window.sessionStorage.getItem('js_deprecation_log_test.warnings'),
+      callback,
+    );

We can't use an arrow function here. So I excluded this line from linting

Lots of errors like this reported by DrupalCI:

space/jenkins-drupal_patches-12602/source/core/misc/drupal.es6.js ✗ 1 more
line 0	File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
/var/lib/drupalci/workspace/jenkins-drupal_patches-12602/source/core/misc/drupal.js ✗ 1 more
0	File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
/var/lib/drupalci/workspace/j

Ran nightwatch tests locally. They do not pass. 😭 I'm getting a mysterious error. It passes for @lauriii and @tedbow. So we got on a call together, and discovered this:

• On DrupalCI, #58:
  

  00:58:05.336 [Js Deprecation Test] Test Suite
  00:58:05.336 ================================
  00:58:10.629 ✔ Element <input[name="modules[js_deprecation_test][enable]"]> was visible after 542 milliseconds.
  00:58:11.947 Running:  Test JavaScript deprecations
  00:58:11.947 
  00:58:12.046 ✔ Element <body> was visible after 26 milliseconds.
  00:58:12.077 ✔ Testing if element <h1> contains text: "JsDeprecationTest"  - 30 ms.
  00:58:12.084 ✔ Testing if "This function is deprecated for testing purposes." deprecation error has been triggered  - 7 ms.
  00:58:12.091 ✔ Testing if "This property is deprecated for testing purposes." deprecation error has been triggered  - 5 ms.
  00:58:12.094 
  00:58:12.094 OK. 4 assertions passed. (147ms)
  

• And #66:
  

  00:58:26.141 [Js Deprecation Test] Test Suite
  00:58:26.141 ================================
  00:58:31.361 ✔ Element <input[name="modules[js_deprecation_test][enable]"]> was visible after 539 milliseconds.
  00:58:32.666 Running:  Test JavaScript deprecations
  00:58:32.666 
  00:58:32.760 ✔ Element <body> was visible after 25 milliseconds.
  00:58:32.791 ✔ Testing if element <h1> contains text: "JsDeprecationTest"  - 30 ms.
  00:58:32.804 
  00:58:32.804 OK. 2 assertions passed. (138ms)
  

  2 assertions, should be 4! This is also happening for Lauri & Ted.

Ted is momentarily going to upload a new patch that fixes the problem!

+++ b/core/tests/Drupal/Nightwatch/Assertions/noDeprecationErrors.js
@@ -16,8 +16,8 @@ module.exports.assertion = function() {
-    this.api.execute(
-      () => window.sessionStorage.getItem('js_deprecation_log_test.warnings'),
-      callback,
-    );
+    // eslint-disable-next-line prefer-arrow-callback
+    this.api.execute(function() {
+      return window.sessionStorage.getItem('js_deprecation_log_test.warnings');
+    }, callback);

This exact change needed to happen in core/tests/Drupal/Nightwatch/Assertions/deprecationErrorExists.js also

RTBC confirmed, #68 passes locally for me 🥳

Committed 1b0c006 and pushed to 9.0.x. Thanks!

c/p to 8.8 and 8.9

Oh: reminder - can we get those documentation updates please kind folks?

piles on as the third committer asking for docs updates

Yes please!

Also, this and the original new coding standard for JS deprecations should go in the release notes (together).

@larowlan & @xjm: I already did that more than a week ago, see #51 :)

EDIT: ah, @catch tagged this needs documentation updates in #61, 4 days after I already did it. That's why the tag was still here.

https://www.drupal.org/core/deprecation#javascript actually still is incomplete, so setting NW so that we don't lose track of it.