Closed Bug 812859 Opened 12 years ago Closed 11 years ago

A JavaScript module to mark deprecation

Categories

(Toolkit :: General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: Yoric, Assigned: yzen)

References

Details

(Whiteboard: [mentor=Yoric][lang=js])

Attachments

(1 file, 5 obsolete files)

As we keep refactoring existing APIs, either to fix bugs or to make them asynchronous, we produce plenty of deprecated APIs. At the moment, we do not have a good/uniform manner of representing deprecation. I believe that we could and should add one.
I envision the feature as a module Deprecated.jsm, with the following signature:

const Deprecated = {
  // A property that dictates whether deprecation warnings should be displayed.
  // It may also be set through property toolkit.deprecation.warnings.on
  get on() {
    // ...
  },
  set on() {
    // ...
  },
  // If |Deprecated.on|, displays on the error console:
  // - "DEPRECATION WARNING: "
  // - the text passed as argument
  // - |stack| if provided
  // - or, if |stack| is not provided, the current JavaScript stack.
  // Otherwise, noop
  warning: function(text, /*optional*/ stack) {
    // ...
  }
};

Ideally, our unit tests will be adapted to orange in case of deprecation warning.
I should add that I would like to make use of this module for OS.File and the synchronous fallbacks of nsSearchService, the Session Store, and more in the future.
Flags: needinfo?(dtownsend+bugmail)
Definitely +1 on on the general idea -- for deprecation to be effective, we need to be able to make it really easy for addon authors to know when they're using something that's going to go away. Online docs and code comments are not effective, and random/inconsistent console logging is too (ie, it need to be consistently used, advertised, and have a way to make it extra-noisy for testing).

A slight concern to the contrary is that to _really_ be effective, we'd probably need a way for the AMO validation tools to scan for deprecated functions. (In the same way they already scan for "unsafe" code.) Developers and users are not always the most rigorous of testers, especially for code that's seldom executed (first-run setup, non-default actions, etc.)

But a simple module sounds like a fine start, and shouldn't be much effort at all.
Whiteboard: [mentor=Yoric][lang=js]
I would suggest an additional argument, a compulsory URL that we include in the output message which should contain information about the deprecation and what to use instead.

Why would we want to turn deprecation warnings on or off at runtime?
Flags: needinfo?(dtownsend+bugmail)
> I would suggest an additional argument, a compulsory URL that we include in the output message which should contain information about the deprecation and what to use instead.

Does this mean that we would need to put a page on MDN for every deprecation? This looks a little overkill.

> Why would we want to turn deprecation warnings on or off at runtime?
I was thinking of doing this for debugging purposes, from the console, but this is probably not necessary in a first version.
(In reply to David Rajchenbach Teller [:Yoric] from comment #5)
> > I would suggest an additional argument, a compulsory URL that we include in the output message which should contain information about the deprecation and what to use instead.
> 
> Does this mean that we would need to put a page on MDN for every
> deprecation? This looks a little overkill.

MDN, or the add-ons blog. We already do this (or should be). Deprecation warnings are no good to developers if they don't come with information on what to use instead.
Assignee: nobody → yura.zenevich
I'd like to take this bug.
This is a first patch to get initial feedback for the issue.

Warning currently reports error if the URL is not provided. Please, let me know if that should just be included as a log message.

I was also wondering if you wanted me to apply this module anywhere?
Attachment #694056 - Flags: review?(dteller)
Comment on attachment 694056 [details] [diff] [review]
Patch that adds Deprecated module to mark deprecated funcionality (Tests included).

Review of attachment 694056 [details] [diff] [review]:
-----------------------------------------------------------------

Quite nice. I think it needs a few changes, but I like it, and I like your tests a lot.

::: toolkit/content/Deprecated.jsm
@@ +14,5 @@
> + *
> + * @param nsIStackFrame aStack or a custom callstack
> + *        A callstack to be parsed into a string log message.
> + */
> +function parseCallstack (aStack) {

I wouldn't call this "parse". Maybe "stringify"?

@@ +28,5 @@
> +}
> +
> +/**
> + * Deprecated is a module that should be used to mark and notify
> + * about deprecation in a uniform matter.

Nit: remove "in a uniform matter".

@@ +50,5 @@
> +
> +    // If URL is not provided, report an error to the error console.
> +    if (!aUrl) {
> +      Components.utils.reportError(
> +        "URL describing deprecation is not provided"

I would be more explicit: "Error in Deprecated.warning: warnings must provide a URL documenting this deprecation", or something such.

@@ +51,5 @@
> +    // If URL is not provided, report an error to the error console.
> +    if (!aUrl) {
> +      Components.utils.reportError(
> +        "URL describing deprecation is not provided"
> +      );

Let's be more violent and also throw a TypeError.

@@ +54,5 @@
> +        "URL describing deprecation is not provided"
> +      );
> +      return;
> +    }
> +

I realize that I have been unclear over IRC. This method should bailout if some preference is unset or set to |false| - say "toolkit.deprecation.warnings".

@@ +55,5 @@
> +      );
> +      return;
> +    }
> +
> +    msg += "\nPlease visit " + aUrl + "\n";

"You may find more details about this deprecation at: "

@@ +61,5 @@
> +    // Append a callstack part to the deprecation message.
> +    msg += parseCallstack(aStack || Components.stack);
> +
> +    // Log deprecation warning.
> +    consoleService.logStringMessage(msg);

Ok, let's refine this to a warning.

::: toolkit/content/tests/browser/browser_Deprecated.js
@@ +17,5 @@
> +  },
> +  expectedObservation: function (aMessage) {
> +    ok(aMessage.message.indexOf("DEPRECATION WARNING: ") === 0,
> +      "Deprecation is correctly logged.");
> +    ok(aMessage.message.indexOf("Please visit http://example.com") > 0,

Just in case messages change or are localized some day, I would just search the URL, without "Please visit"

@@ +21,5 @@
> +    ok(aMessage.message.indexOf("Please visit http://example.com") > 0,
> +      "URL is correctly logged.");
> +    ok(aMessage.message.indexOf(
> +      "browser_Deprecated.js 15 tests<.deprecatedFunction") > 0,
> +      "Callstack is correctly logged.");

While you are at it, you might also search for "this method is deprecated." in |aMessage|. In this case, don't forget to change the message for each test.

@@ +33,5 @@
> +  },
> +  expectedObservation: function (aMessage) {
> +    if (!(aMessage instanceof Ci.nsIScriptError)) {
> +      ok(false, "Deprecated warning should log an error if URL argument " +
> +        "is not passed");

Nice
Attachment #694056 - Flags: review?(dteller) → feedback+
Thanks for the comments! They should now be addressed.

Unfortunately I was not able to use Console.jsm API's warning, as it does something different (dump) from logging a nsIScriptError with a warning flag.
Attachment #694056 - Attachment is obsolete: true
Attachment #694545 - Flags: review?(dteller)
I also wonder if I should declare "toolkit.deprecation.warnings" in the list of prefs?
Thanks.
Comment on attachment 694545 [details] [diff] [review]
Updated patch that addresses the code review.

Review of attachment 694545 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, with a few minor changes. As I will be away for 2 weeks (and as I am not a peer of toolkit/), you should ask someone else to review further versions of your patch, Dolske or Mossop, for instance.

::: toolkit/content/Deprecated.jsm
@@ +10,5 @@
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const consoleService = Cc["@mozilla.org/consoleservice;1"]
> +  .getService(Ci.nsIConsoleService);

Since you are importing Services, you can just do |Services.console|.

@@ +49,5 @@
> +   *        snapshot of the current JavaScript callstack will be
> +   *        logged.
> +   */
> +  warning: function (aText, aUrl, aStack) {
> +    let consoleMessage, textMessage;

Nit: Generally, we prefer declaring variables as close as possible to where they are used, at least when they are |let| variables.

@@ +55,5 @@
> +    // Only log deprecation warning if toolkit.deprecation.warnings
> +    // preference is set.
> +    if (!Services.prefs.getBoolPref("toolkit.deprecation.warnings")) {
> +      return;
> +    }

That's going to be pretty slow. You should rather have a variable |on| (or something such), that is initialized from "toolkit.deprecation.warnings", and a preference observer for "toolkit.deprecation.warnings" that will change the value of |on| whenever necessary.

@@ +61,5 @@
> +    // If URL is not provided, throw an error.
> +    if (!aUrl) {
> +      throw new TypeError(
> +        "Error in Deprecated.warning: warnings must provide a URL " +
> +        "documenting this deprecation."

In some contexts, errors are caught and ignored. I would prefer having both this TypeError and a Cu.reportError

@@ +74,5 @@
> +    // Log deprecation warning.
> +    consoleMessage = Cc["@mozilla.org/scripterror;1"]
> +      .createInstance(Ci.nsIScriptError);
> +    consoleMessage.init(textMessage, null, null, 0, 0,
> +      Ci.nsIScriptError.warningFlag, "content javascript");

"content javascript"? That sounds weird. Maybe you meant "chrome javascript"?
Attachment #694545 - Flags: review?(dteller) → feedback+
Hi Justin,
David mentioned that you would be able to take a look at my patch given that he is away for the next 2 weeks. That would be great if you can :).
Thanks
Attachment #694545 - Attachment is obsolete: true
Attachment #695097 - Flags: review?(dolske)
Comment on attachment 695097 [details] [diff] [review]
Updating the patch according to latest comments.

Review of attachment 695097 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/Deprecated.jsm
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Hmm, toolkit/content/ is becoming a bit of a clutterpile, but let's not worry about that here.

@@ +6,5 @@
> +
> +this.EXPORTED_SYMBOLS = [ "Deprecated" ];
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +const PREF_DEPRECATION_WARNINGS = "toolkit.deprecation.warnings";

"Toolkit" is an implementation detail, let's not expose that in the name.

I'd suggest devtools.errorconsole.deprecation_warnings (since from the user's point of view, it's a thing that shows up in the error console).

@@ +11,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +// A flag that indicates whether deprecation warnings should be logged.
> +let deprecationOn = Services.prefs.getBoolPref(PREF_DEPRECATION_WARNINGS);

It's not deprecation that's being turned on/off, it's the logging... s/deprecationOn/logWarnings/

@@ +23,5 @@
> +   */
> +  observe: function (aSubject, aTopic, aData) {
> +    deprecationOn = Services.prefs.getBoolPref(PREF_DEPRECATION_WARNINGS);
> +  }
> +};

This should just magically work as a plain function (thanks to nsIObserver's [function] tag)...

  function deprecationPrefObserver(aSubject, aTopic, aData) {
    logWarnings = Services.prefs.getBoolPref(PREF_DEPRECATION_WARNINGS);
  }

@@ +50,5 @@
> +
> +/**
> + * Deprecated is a module that should be used to mark and notify
> + * about deprecation.
> + */

This comment doesn't seem very useful. I'd just remove it.

@@ +78,5 @@
> +      let errorMessage = "Error in Deprecated.warning: warnings must " +
> +        "provide a URL documenting this deprecation."
> +      Cu.reportError(errorMessage);
> +      throw new TypeError(errorMessage);
> +    }

This shouldn't throw -- it's not actually fatal, and if someone forgets/bungles the URL this will cause things to break.

Fine to make it noisy and complain with a message, though.

@@ +83,5 @@
> +
> +    let textMessage = "DEPRECATION WARNING: " + aText + "\nYou may find more " +
> +      "details about this deprecation at: " + aUrl + ".\n" +
> +      // Append a callstack part to the deprecation message.
> +      stringifyCallstack(aStack || Components.stack);

Something should check for |aStack instance of Ci.nsIStackFrame|.

I'd suggest doing that in stringifyCallstack, along with a fallback to using Components.stack if aStack is bogus or null.

@@ +89,5 @@
> +    // Log deprecation warning.
> +    let consoleMessage = Cc["@mozilla.org/scripterror;1"]
> +      .createInstance(Ci.nsIScriptError);
> +    consoleMessage.init(textMessage, null, null, 0, 0,
> +      Ci.nsIScriptError.warningFlag, "chrome javascript");

Ugh. Our APIs here suck. I hate that we're using nsIScriptError here, but can't actually provide a useful file / line / column. Service.console.logStringMessage() would be more humane, but those only show up as "messages" instead of warnings.

[Actually, can we get that info from the stack? Seems like we can? That would probably be more useful that dumping the whole callstack.]

Maybe we should just Cu.reportError() here too. Warnings will just get lost anyway in the usual flood of CSS warning.
Attachment #695097 - Flags: review?(dolske) → review-
Attached patch Addressed the latest comments. (obsolete) — Splinter Review
All the comments should be addressed in this patch.
As far as I understood, you suggested to use Cu.reportError to both: complain about missing URL as well as the actual deprecation message? Hopefully that's what you meant.
Attachment #695097 - Attachment is obsolete: true
Attachment #695926 - Flags: feedback?(dolske)
Comment on attachment 695926 [details] [diff] [review]
Addressed the latest comments.

Review of attachment 695926 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, noticed a couple more things but you're on the right track!

::: toolkit/content/Deprecated.jsm
@@ +12,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +// A flag that indicates whether deprecation warnings should be logged.
> +let logWarnings = Services.prefs.getBoolPref(PREF_DEPRECATION_WARNINGS);

Hmm, I think you've got a subtle bug here. This should throw when the pref doesn't have a default value. (Is this even working if you try using this module from real browser code? I bet it isn't.)

In any case, we should just add the pref to modules/libpref/src/init/all.js with a default value of true. That fixes the bug, and avoids the need to handle a not-default-value case.

@@ +16,5 @@
> +let logWarnings = Services.prefs.getBoolPref(PREF_DEPRECATION_WARNINGS);
> +
> +// Add an observer listening to changes to
> +// devtools.errorconsole.deprecation_warnings that enables or disables the
> +// deprecation logging.

I'd avoid referring to the pref by name, since that defeats the 3/11ths of the reason for using a const for it. :)

The code here's pretty obvious, so I'd just delete this comment entirely.

@@ +20,5 @@
> +// deprecation logging.
> +Services.prefs.addObserver(PREF_DEPRECATION_WARNINGS,
> +  function (aSubject, aTopic, aData) {
> +    logWarnings = Services.prefs.getBoolPref(PREF_DEPRECATION_WARNINGS);
> +  }, false);

Oh, nice. Anonymous function ftw.

@@ +36,5 @@
> +  }
> +
> +  let frame = aStack.caller;
> +  let msg = "";
> +  // Get every frame in the callstack.

If we end up using |Components.stack|, probably ought to ignore the last 2 frames, since they'll always be internal to this module.

@@ +61,5 @@
> +   *        logged.
> +   */
> +  warning: function (aText, aUrl, aStack) {
> +    // Only log deprecation warning if
> +    // devtools.errorconsole.deprecation_warnings preference is set to true.

Ditto to above. Suggest just removing.

@@ +74,5 @@
> +      return;
> +    }
> +
> +    let textMessage = "DEPRECATION WARNING: " + aText + "\nYou may find more " +
> +      "details about this deprecation at: " + aUrl + ".\n" +

Nit: you could avoid a bit of garbage and have nicer formatting:

  let textMessage = "DEPRECATION WARNING: " + aText +
    "\nYou may find more details about this deprecation at: " +
    aUrl + "\n" +
    stringifyCallstack(aStack);

Nit2: (note that I removed the period that was appended right after aUrl, just to make cut'n'paste less likely to accidentally append a bogus "." to the url if the user tries to load it.)

::: toolkit/content/tests/browser/browser_Deprecated.js
@@ +8,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm", this);
> +
> +// Initially set devtools.errorconsole.deprecation_warnings pref to false.
> +Services.prefs.setBoolPref(PREF_DEPRECATION_WARNINGS, false);

Nuke comment. Also, this masking the aforementioned bug. Better to test with the default value first (no explicit pref setting), and then set to !default and test that after.

@@ +43,5 @@
> +{
> +  deprecatedFunction: basicDeprecatedFunction,
> +  expectedObservation: function (aMessage) {
> +    // Nothing should be logged when devtools.errorconsole.deprecation_warnings
> +    // is falsy.

// Nothing should be logged when perf is false

@@ +45,5 @@
> +  expectedObservation: function (aMessage) {
> +    // Nothing should be logged when devtools.errorconsole.deprecation_warnings
> +    // is falsy.
> +    ok(false, "Deprecated warning should not log anything when " +
> +        "devtools.errorconsole.deprecation_warnings is unset.");

"... when pref is unset ..."

@@ +125,5 @@
> +  let consoleListener = {
> +    observe: test.expectedObservation
> +  };
> +  // Register a listener that contains the tests.
> +  Services.console.registerListener(consoleListener);

Hmm. This patten has caused intermittent failures in the past... Consider what happens when other random code (due to, say, GC happening) logs some messages in the middle of your test run.

You should either:

a) make your observers insensitive to firing repeatedly (ie, ignore unexpected messages, only continue test when expected mesaage arrives)

or

b) Just use Services.console.getMessageArray(), since I think this is all logged synchronously, and loop over everything to check that your expected message in in there somewhere.
Attachment #695926 - Flags: feedback?(dolske) → feedback+
Attached patch Addressed latest comments. (obsolete) — Splinter Review
Thanks for the comments. I looked through the logs in tests and it seems like I only need to remove the last frame when using Components.stack.
Attachment #695926 - Attachment is obsolete: true
Attachment #696112 - Flags: review?(dolske)
Comment on attachment 696112 [details] [diff] [review]
Addressed latest comments.

Hmm, I think you missed the very end of comment 17 about the test being failure-prone? Everything else looks fine, though.
Attachment #696112 - Flags: review?(dolske) → review-
Thanks for catching that. I missed your comment in the test file indeed.
Attachment #696112 - Attachment is obsolete: true
Attachment #696223 - Flags: feedback?(dolske)
Comment on attachment 696223 [details] [diff] [review]
Updated tests according to comments.

Thanks!
Attachment #696223 - Flags: feedback?(dolske) → feedback+
Comment on attachment 696223 [details] [diff] [review]
Updated tests according to comments.

Rather, r+.
Attachment #696223 - Flags: feedback+ → review+
Hi Justin, is there anything I should take a look at in regards to this bug e.g. make use of it somewhere, as David mentioned (unless that belongs in a different ticket)?
Thanks.
Flags: needinfo?(dolske)
No, I suggest landing this as-is, and file new bugs to start using it where appropriate.
Flags: needinfo?(dolske)
I agree with Justin.
Yura, are we waiting for something or is the patch ready to land?
Flags: needinfo?(yura.zenevich)
I believe it is ready (since Justin gave the last revision an r+).
Flags: needinfo?(yura.zenevich)
By the way, yzen, next time, could you add a commit message to your patch, as per the guidelines at http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed ?
Ah, I just realized that I set myself as author of the patch by accident. My apologies. If you had set yourself as author, this wouldn't have happened :)
https://hg.mozilla.org/mozilla-central/rev/a45739b82322
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
BTW, we have toolkit/modules now, which would have been a moderately better place to put this, I think. But we can fix that in bug 828116.
You need to log in before you can comment on or make changes to this bug.