[RFC 297] Deprecation of Ember.Logger #16250

lupestro · 2018-02-16T15:05:17Z

I've implemented the changes to the project. Since Ember itself no longer uses Ember.Logger, this PR could perhaps use one more test that deliberately calls the methods to verify that the console is reporting deprecation. Otherwise, this should be set to go.

lupestro · 2018-02-17T02:48:18Z

Four tests failed because the change I made to the debug() method in ember-debug/lib/index.js didn't fare well in Edge and IE11. I'll hop on Windows and nail it down in the morning.

lupestro · 2018-02-17T20:11:44Z

This has been interesting. There are 20 tests that fail only in Edge and IE11- and only when you don't have the developer tools up. The point of failure lies in precisely two lines of code in the source.

The first occurs with (console.debug ? console.debug : console.log)(params), which was easy to work around by replacing the operator with an actual if/else, but the workaround only fixes one test case. The failure gave a pretty strong hint of at least part of what's going on - when Edge isn't in devtools, I think we're dealing with a bound member function of an object - but I need to investigate further.

The second occurs doing console.error.apply(this, errorArgs) in one spot in the router, and is responsible for at least 18 and probably all 19 of the remaining cases. I can shim around that by wrapping a call to console.error in an un-exported tiny logger shim class for the router only to use. With a little deeper understanding, I can probably do better still, and have a great topic for a blog post from it, so I'm going to pursue that. However, I'll do the shim first, since I know that will work.

lupestro · 2018-02-17T22:03:01Z

Someone please review packages\ember-routing\lib\system\router.js line 1193 - I debated between ease of understanding and absolute performance here - should it be yanked to the top of the module to be created once? How critical is that? I'll still chase something more elegant once I figure out how Edge is defining the console differently with devtools closed, but this approach turns clearly failing tests to consistently passing tests, at least on my Windows box.

locks · 2018-02-19T13:41:06Z

packages/ember-console/lib/index.js

+  deprecate(
+    'Use of Ember.Logger is deprecated. Please use `console` for logging.',
+    false,
+    { id: 'ember-console.deprecate-logger', until: '3.5.0', url: 'https://emberjs.com/deprecations/v3.x#toc_use-console-rather-than-ember-logger' }


I think the until should be 4.0.0.

rwjblue · 2018-02-24T03:13:58Z

packages/ember-console/lib/index.js

+import { deprecate } from 'ember-debug';
+
+// Deliver message that the function is deprecated
+let deprecateEmberLogger = function(){


I definitely appreciate the desire to be a bit DRY here, but the current implementation has a larger footprint in production builds (where the deprecation is stripped). Would you mind inlining the deprecation in each of the methods instead (I am sorry that we can’t have nice things...)?

See if you like the approach I just checked in - I've split the difference to ensure that repeatedly typing long strings can't result in unnecessary and hard-to-spot differences, while still calling deprecate clearly in each method.

rwjblue · 2018-02-24T03:15:38Z

packages/ember-debug/lib/index.js

@@ -130,7 +136,9 @@ if (DEBUG) {
    @private
  */
  setDebugFunction('info', function info() {
-    Logger.info.apply(undefined, arguments);
+    /* eslint-disable no-console */
+    console.info.apply(console, arguments); 


Can we do console.info(...arguments) here instead?

rwjblue · 2018-02-24T03:16:54Z

packages/ember-debug/lib/index.js

@@ -130,7 +136,9 @@ if (DEBUG) {
    @private
  */
  setDebugFunction('info', function info() {
-    Logger.info.apply(undefined, arguments);
+    /* eslint-disable no-console */


@for some of these disablings you may be able to make it a tad less noisy via // eslint-disable-next-line no-console

rwjblue · 2018-02-24T03:17:58Z

packages/ember-debug/lib/warn.js

-    }
+    /* eslint-disable no-console */
+    console.warn(`WARNING: ${message}`); //eslint-disable-line no-console
+    // VERIFY! if ('trace' in Logger) {


Can probably kill this comment

rwjblue · 2018-02-24T03:18:31Z

packages/ember-debug/lib/warn.js

-      Logger.trace();
-    }
+    /* eslint-disable no-console */
+    console.warn(`WARNING: ${message}`); //eslint-disable-line no-console


Seems like this comment isn’t needed (since the surrounding ones should do the trick)

rwjblue · 2018-03-02T21:58:09Z

packages/ember-routing/lib/system/router.js

@@ -106,7 +105,13 @@ const EmberRouter = EmberObject.extend(Evented, {

    if (DEBUG) {
      if (get(this, 'namespace.LOG_TRANSITIONS_INTERNAL')) {
-        routerMicrolib.log = Logger.debug;
+        /* eslint-disable no-console */
+        if (console.debug) {


lets remove this, and just use console.log version only

rwjblue · 2018-03-02T21:58:36Z

packages/ember-routing/lib/system/router.js

@@ -1181,7 +1189,7 @@ function logError(_error, initialMessage) {
    if (typeof error === 'string') { errorArgs.push(error); }
  }

-  Logger.error.apply(this, errorArgs);
+  console.error.apply(console, errorArgs); //eslint-disable-line no-console


can you update this to console.error(...errorArgs) instead?

rwjblue · 2018-03-02T21:59:54Z

packages/ember/tests/routing/basic_test.js

-
-      originalLoggerError = Logger.error;
+      
+      originalLoggerError = console.error; // eslint-disable-line no-console


Can you also update the variable name here (so it doesn't refer to logger)?

rwjblue · 2018-03-02T22:01:47Z

packages/ember-glimmer/lib/helpers/log.ts

-  Logger.log.apply(null, positional.value());
-}
+    /* eslint-disable no-console */
+    console.log.apply(console, positional.value());


can you modify to console.log(...positional.value())?

lupestro · 2018-03-02T22:45:49Z

Sorry for the big stack of stuff - shouldn't be much actual change - I sync'd up my fork/master and rebased the branch... no conflicts reported and everything tested out fine - this should have everything you requested...

addressed feedback

rwjblue · 2018-03-09T21:18:29Z

🎉

rwjblue · 2018-03-09T21:18:45Z

Thanks @lupestro for all the hard work on this!

locks previously requested changes Feb 19, 2018

View reviewed changes

lupestro mentioned this pull request Feb 19, 2018

[RFC 297] Deprecation Ember.Logger #16231

Closed

8 tasks

rwjblue requested changes Feb 24, 2018

View reviewed changes

rwjblue requested changes Mar 2, 2018

View reviewed changes

locks force-pushed the deprecate-ember-logger branch from cc425c7 to 4bbb29b Compare March 3, 2018 22:48

rwjblue approved these changes Mar 9, 2018

View reviewed changes

lupestro added 2 commits March 9, 2018 15:43

Deprecated Ember Logging

ffd43ca

Using latest eslint-plugin-ember-internal 1.1.1

da5b58e

rwjblue force-pushed the deprecate-ember-logger branch from 4bbb29b to da5b58e Compare March 9, 2018 20:44

rwjblue merged commit 76dc4de into emberjs:master Mar 9, 2018

lupestro deleted the deprecate-ember-logger branch March 13, 2018 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC 297] Deprecation of Ember.Logger #16250

[RFC 297] Deprecation of Ember.Logger #16250

lupestro commented Feb 16, 2018

lupestro commented Feb 17, 2018

lupestro commented Feb 17, 2018

lupestro commented Feb 17, 2018

locks Feb 19, 2018

rwjblue Feb 24, 2018

lupestro Feb 24, 2018

rwjblue Feb 24, 2018

rwjblue Feb 24, 2018

rwjblue Feb 24, 2018

rwjblue Feb 24, 2018

rwjblue Mar 2, 2018

rwjblue Mar 2, 2018

rwjblue Mar 2, 2018

rwjblue Mar 2, 2018

lupestro commented Mar 2, 2018

rwjblue commented Mar 9, 2018

rwjblue commented Mar 9, 2018


		originalLoggerError = Logger.error;

		originalLoggerError = console.error; // eslint-disable-line no-console

[RFC 297] Deprecation of Ember.Logger #16250

[RFC 297] Deprecation of Ember.Logger #16250

Conversation

lupestro commented Feb 16, 2018

lupestro commented Feb 17, 2018

lupestro commented Feb 17, 2018

lupestro commented Feb 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lupestro commented Mar 2, 2018

rwjblue commented Mar 9, 2018

rwjblue commented Mar 9, 2018