Skip to content
This repository has been archived by the owner on Mar 22, 2019. It is now read-only.

RFC#297 | Ember.Logger deprecation #3197

Merged
merged 7 commits into from Mar 2, 2018

Conversation

lupestro
Copy link

This is the deprecation text to go with RFC 297 ("Deprecation of Ember.Logger") currently a PR on emberjs/ember.js. I'm entertaining feedback on the text now, but it shouldn't be merged until that PR is also merged.

locks
locks previously requested changes Feb 19, 2018
@@ -53,3 +53,42 @@ object.notifyPropertyChange('someProperty');
##### id: ember-metal.getting-each

Calling `array.get('@each')` is deprecated. `@each` may only be used as dependency key.

#### Use console rather than Ember.Logger
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This deprecation should be under "upcoming deprecation" as it is not included in a release as of yet :)

For example, the following uses will not work in Edge and IE11 except when the development tools window is open:

``` javascript
var print = console.log; // print is unbound, therefore bound to global
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this mentioned? Seems unrelated to the deprecation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the user changes their code to silence the deprecation, it will at first appear that a simple text substitution of “Logger”=>”console” will do. However, the moment anybody runs their code with Edge or IE11 without the debugger open, they’ll face-plant into this issue. The solution is simple but non-obvious, so I figured I’d better document it so anybody who is updating their code by hand will be equipped to handle it as they make their change. This is something the codemod will also need to deal with or flag for manual intervention.

@locks locks changed the title Deprecation text to go with deprecation source RFC#297 | Ember.Logger deprecation Feb 19, 2018
to:

``` javascript
console.info.apply(console, arguments);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on console.info(...arguments)?

@sivakumar-kailasam
Copy link
Member

@lupestro We're migrating deprecations from this repo into its own app. Can you please raise a new PR following these instructions?

@lupestro
Copy link
Author

lupestro commented Feb 21, 2018

@sivakumar-kailasam
Copy link
Member

Thanks @lupestro closing this in favor of the new PR.

@@ -53,3 +53,57 @@ object.notifyPropertyChange('someProperty');
##### id: ember-metal.getting-each

Calling `array.get('@each')` is deprecated. `@each` may only be used as dependency key.

### Deprecations Added in Upcoming Versions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the conflicting advice, can you change this to be within the 3.2 section? I'm pretty sure we will be able to tackle it before 3.2 goes to beta...


The `Ember.Logger` module will be removed at the start of the 4.x cycle. You should remove any calls to `Ember.Logger` and use calls to `console` instead.

In Edge and IE11, uses of `console` beyond calling its methods may require more subtle changes than simply substituting `console` wherever `Logger` appears. In these browsers, they will behave just as they do in other browsers when your development tools window is open. However, when run normally, calls to its methods must not be bound to anything other than the console object or you will receive an `Invalid calling object` exception. This is a known inconsistency with these browsers. (Edge issue #14495220.)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The various caveats should be under a specific section (e.g. Migration Gotchas or somesuch). I see two main gotchas here:

  • calling console methods unbound
  • console.debug being missing

FWIW, I think the specific guidance for the latter item there is to just not use console.debug...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The section headings at this level aren't terribly distinct, but I broke it up under headings. Looking at a lot of the other deprecations in 3.x and 2.x, the description for this one is a bit more involved, but I think the detail is necessary.

##### until: 4.0.0
##### id: ember-console.deprecate-logger

The `Ember.Logger` module will be removed at the start of the 4.x cycle. You should remove any calls to `Ember.Logger` and use calls to `console` instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Directly under this line, can you show a few before and after examples?

I'm thinking something like:

// Before
Ember.Logger.log('my little pony');

// After
console.log('my little pony');

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. While I was at it, I combined the before/after pairs down on the other examples. Hopefully that doesn't violate editorial conventions too badly. They do read better with the code lines closer together and, with three of them, it saves significant space and plays down the changes a little.

@lupestro
Copy link
Author

lupestro commented Mar 1, 2018

When the editing is complete and this is merged, I'll migrate the content over to deprecation-app as well, so it's there for the future.

@rwjblue rwjblue merged commit d790ce6 into emberjs:master Mar 2, 2018
@lupestro lupestro deleted the deprecate-ember-logger branch March 22, 2018 23:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants