Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecating Ember.Logger #297

Merged
merged 9 commits into from Mar 2, 2018

Conversation

@lupestro
Copy link
Contributor

commented Jan 19, 2018

This is the RFC for the work to deprecate Ember.Logger.

Rendered.

lupestro and others added 5 commits Jan 18, 2018
First draft of RFC to deprecate the Ember.Logger
RFC to deprecate the Ember.Logger that currently lives in ember-console. 
I'm new to this process and this is a preliminary draft, so please be gentle. :-}
@mmun

This comment has been minimized.

Copy link
Member

commented Jan 19, 2018

I support this change. Happy to help implement as well.

@mmun mmun added the T-framework label Jan 19, 2018

@ef4

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2018

This is good, and it looks like a nicely separable piece of work that doesn't require any deep experience with ember internals.

their own logging service. If we encourage the use of a Babel plugin
to strip console calls, should we remove `no-console` from the default
flags that `ember-cli` ships?
How do we deal with `Logger.debug` in the codemod?

This comment has been minimized.

Copy link
@locks

locks Jan 19, 2018

Contributor

A suggestion: Try making the question a sub-header and not use bullet points for all of the answer.

`Logger.warn`, so the `ember-debug` code should be changed _first_ or adding
the deprecation warning will create a deep recursion.

The log and debug messages emitted by Ember for deprecations and assertions

This comment has been minimized.

Copy link
@locks

locks Jan 19, 2018

Contributor

You might want to slightly rephrase this section to specifically indicate that it is Ember.warn and friends that are noops in production.

@paulyoder

This comment has been minimized.

Copy link

commented Jan 26, 2018

What happens in the situation where the Ember.Logger methods are wrapped in another function to provide additional functionality?

For example, my app has an initializer that sends all log messages to Rollbar. Here is what it looks like for the Ember.Logger.info method:

let infoLogger = Ember.Logger.info;
Ember.Logger.info = function() {
  Rollbar.info.apply(Rollbar, arguments);
  infoLogger.apply(this, arguments);
};

How would I change this code in light of this RFC? Would I just override console.info directly then?

@bendemboski

This comment has been minimized.

Copy link

commented Jan 26, 2018

@paulyoder I would think you'd just override console.info directly -- that's what many libraries that don't depend on a framework like Ember do, e.g. TrackJS.

@bendemboski

This comment has been minimized.

Copy link

commented Jan 26, 2018

I think this makes sense. As far as the eslint question, I personally wouldn't want to turn off no-console because I frequently scatter console.log()s and console.count()s and whatnot in my code while I'm debugging something, and want eslint to remind me to remove them before checking in the code. So I would opt for something like this:

no-console: ["error", { allow: ["info", "warn", "error"] }]

This gives me a good set of options for logging to the console, but also allows me to use console.log() or console.debug() (and various other console methods) with the eslint safety net.

Personally, I'd advocate for something like ☝️ over just disabling no-console in ember-cli's blueprints, although I'm uncertain of the merits of doing ☝️ over just leaving it on and letting the user decide.

@webark

This comment has been minimized.

Copy link

commented Jan 27, 2018

I think that separate from this RFC, it would be nice to have another RFC that deals with the "debugging and logging story" of ember. I think this is where a lot of these kinds of issues could come out, and you could tap into some parts of ember and other extensible tools to get a richer logging and debugging experience.

@locks

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2018

@webark send that RFC over ;)

@rwjblue

This comment has been minimized.

Copy link
Member

commented Feb 4, 2018

@lupestro - Great job putting this together!!

I only have a few responses to the unanswered questions section:

How do we deal with Logger.debug in the codemod?

I think replacing it with (console.debug || console.log)(<whatever arguments>) would be perfectly fine (and be an accurate codemod). I think Logger.debug is fairly rare, and having the codemod roughly guarantee existing functionality is a good default...

What do we do about the eslint no-console flag?

I think we do nothing. Leaving Logger.log's strewn about a codebase just as a way to work around the no-console rule is much worse than simply adding // eslint-disable-line no-console (the codemod could do this for the user automatically also).

@locks locks added this to In Progress in Deprecation Candidates Feb 5, 2018

@rwjblue

This comment has been minimized.

Copy link
Member

commented Feb 9, 2018

We discussed this at the core team meeting today, and we are general happy to move forward on this RFC. Since it seems like conversation has largely settled here, we are moving it into the final comment period.

@locks locks referenced this pull request Feb 9, 2018
7 of 8 tasks complete
Ralph Mack
@machty

This comment has been minimized.

Copy link
Member

commented Feb 23, 2018

I'm the proud maintainer of the addon with the most uses of Ember.Logger (ember-concurrency), and technically e-c supports Embers all the way back to 1.13. If I want to continue that long-term support I'll have to have some kind of code that fulfills a similar purpose as Ember.Logger; what's the proper strategy for me going forward?

@lupestro

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2018

Currently, the best recommendation I can think of is to build a small wrapper of your own, preferably as a service suitable for dependency injection, possibly even available to your clients for interception. (Ember.Logger itself has something like a single functional line for each kind of console call it performs, so your wrapper shouldn't need to be any bigger.)

I'm hoping other reviewers will have a better suggestion. I'll make sure the best suggestion makes it into the deprecation guide, since this is a question that is likely to be asked a lot. Should I say anything in the RFC text at this late date?

@MelSumner

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2018

Was the option 2 considered as a bridge, or will that just prolong the pain?

Extract Ember.Logger into its own (tiny) @ember/console package as a shim for users.

@rwjblue

This comment has been minimized.

Copy link
Member

commented Mar 2, 2018

@MelSumner - I think that is likely fine but under a different name (e.g. @ember/legacy-logger or somesuch).

@machty - I believe that the answer here is more situational (e.g. it depends how an addon uses Ember.Logger). In the ember-concurrency cases, I have specific suggestions below.

  • Warning for not using maxConcurrency - This should be swapped for deprecate (the message explicitly states that it is a deprecation). This change is backwards compatible and avoids any deprecations proposed by this RFC.
  • Logging the cancellation reason when manually debugging - I would change this usage to directly invoke console.log. This would mean that someone could not debug manually on IE < 11, but that seems like an extremely unlikely scenario...
@rwjblue

This comment has been minimized.

Copy link
Member

commented Mar 2, 2018

Thanks to everyone for participating in this RFC! After discussion with the core team, we are 👍 on moving forward here.

@rwjblue rwjblue merged commit 941656e into emberjs:master Mar 2, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Deprecation Candidates automation moved this from In Progress to Finished Mar 2, 2018

@lupestro lupestro deleted the lupestro:lupestro-deprecate-logger branch Mar 22, 2018

@lupestro lupestro restored the lupestro:lupestro-deprecate-logger branch Mar 24, 2018

rwjblue added a commit to rwjblue/ember.js that referenced this pull request Dec 20, 2018
[BUGFIX lts] Avoid console.trace for every Ember.warn
Originally (Ember < 3.2), `Ember.warn` used `Ember.Logger` as an
abstraction layer in case `console` was not present. Since
`Ember.Logger` was deprecated in
[emberjs/rfcs#297](https://emberjs.github.io/rfcs/0297-deprecate-ember-logger.html)
the internals have been refactored to use `console` directly instead.

When that change was made, the following:

```js
    Logger.warn(`WARNING: ${message}`);
    if ('trace' in Logger) {
      Logger.trace();
    }
```

Was changed to:

```js
    console.warn(`WARNING: ${message}`);
    if (console.trace) {
      console.trace();
    }
```

This _seems_ correct, however when you dig into it you will notice that
the `Ember.Logger` class **never** had a `.trace` method! The reason for
the original `'trace' in Logger` check was specifically so that you
_could_ do `Ember.Logger.trace = () => console.trace` _IIF_ you wanted
to see where a given warning was coming from. That was added back in
2012, but since then the developer tools of modern browsers have gotten
massively better. At this point, **every** `console.log`/`console.warn`
tracks its stack trace so that you can drill into the source in the dev
tools. The primary difference between that functionality and calling
`console.trace()` directly like this is that with `console.warn`
the stack trace is hidden by default (and has to be manually expanded),
whereas with `console.trace()` it is _always_ called and the full stack
is printed.

Ultimately, this means that when we refactored to address the
`Ember.Logger` deprecation, we began calling `console.trace` for _every_
`Ember.warn` invocation and the `console.trace()` calls make the console
fairly unusable even with a very low volumn of warnings.
kategengler added a commit to emberjs/ember.js that referenced this pull request Dec 24, 2018
[BUGFIX lts] Avoid console.trace for every Ember.warn
Originally (Ember < 3.2), `Ember.warn` used `Ember.Logger` as an
abstraction layer in case `console` was not present. Since
`Ember.Logger` was deprecated in
[emberjs/rfcs#297](https://emberjs.github.io/rfcs/0297-deprecate-ember-logger.html)
the internals have been refactored to use `console` directly instead.

When that change was made, the following:

```js
    Logger.warn(`WARNING: ${message}`);
    if ('trace' in Logger) {
      Logger.trace();
    }
```

Was changed to:

```js
    console.warn(`WARNING: ${message}`);
    if (console.trace) {
      console.trace();
    }
```

This _seems_ correct, however when you dig into it you will notice that
the `Ember.Logger` class **never** had a `.trace` method! The reason for
the original `'trace' in Logger` check was specifically so that you
_could_ do `Ember.Logger.trace = () => console.trace` _IIF_ you wanted
to see where a given warning was coming from. That was added back in
2012, but since then the developer tools of modern browsers have gotten
massively better. At this point, **every** `console.log`/`console.warn`
tracks its stack trace so that you can drill into the source in the dev
tools. The primary difference between that functionality and calling
`console.trace()` directly like this is that with `console.warn`
the stack trace is hidden by default (and has to be manually expanded),
whereas with `console.trace()` it is _always_ called and the full stack
is printed.

Ultimately, this means that when we refactored to address the
`Ember.Logger` deprecation, we began calling `console.trace` for _every_
`Ember.warn` invocation and the `console.trace()` calls make the console
fairly unusable even with a very low volumn of warnings.

(cherry picked from commit 3e45eb9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.