Fix for bug 786286 - Define our module stability ratings in the SDK docs #654

Merged
merged 9 commits into from Nov 21, 2012

Conversation

Projects
None yet
3 participants

@Mossop Mossop commented on an outdated diff Nov 12, 2012

doc/dev-guide-source/guides/stability.md
@@ -0,0 +1,106 @@
+<!-- This Source Code Form is subject to the terms of the Mozilla Public
+ - License, v. 2.0. If a copy of the MPL was not distributed with this
+ - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
+
+# SDK API Lifecycle #
+
+Developers using the SDK's APIs need to know how far they can trust that
+a given API will not change in future releases. At the same time, developers
+maintaining and extending the SDKs APIs need to be able to introduce new
@Mossop

Mossop Nov 12, 2012

Member

s/SDKs/SDK's/

@Mossop Mossop commented on an outdated diff Nov 12, 2012

doc/dev-guide-source/guides/stability.md
+# SDK API Lifecycle #
+
+Developers using the SDK's APIs need to know how far they can trust that
+a given API will not change in future releases. At the same time, developers
+maintaining and extending the SDKs APIs need to be able to introduce new
+APIs that aren't yet fully proven, and to retire old APIs when they're
+no longer optimal or supported by the underlying platform.
+
+The API lifecycle aims to balance these competing demands. It has two
+main components:
+
+* a [stability index](dev-guide/guides/stability.html#Stability Index)
+that defines how stable each module is
+* a [deprecation process](dev-guide/guides/stability.html#Deprecation Process)
+that is intended to enable the SDK to remove or change APIs when necessary,
+while giving developers enough time to update their code.
@Mossop

Mossop Nov 12, 2012

Member

How about: "that defines when and how stable SDK APIs can be changed or removed from future versions of the SDK while giving developers enough time to update their code."

@Mossop Mossop and 1 other commented on an outdated diff Nov 12, 2012

doc/dev-guide-source/guides/stability.md
+Before deprecating a module, the SDK team will develop and document
+an alternative, and decide which release to deprecate it in.
+
+### Deprecation ###
+
+In the chosen release, the SDK team will communicate the module's deprecation:
+
+* update the module's stability index to be "deprecated"
+* include a deprecation notice in the
+[release notes](https://wiki.mozilla.org/Labs/Jetpack/Release_Notes),
+the [Add-ons blog](https://blog.mozilla.org/addons/), and the
+[Jetpack Google group](https://groups.google.com/forum/?fromgroups#!forum/mozilla-labs-jetpack). The deprecation notice should point developers at a migration guide.
+
+### Migration ###
+
+The deprecation period defaults to 18 weeks (that is, three releases).
@Mossop

Mossop Nov 12, 2012

Member

It might be worth mentioning that in some cases (generally those out of our control) we might need to deprecate faster than this.

@ZER0

ZER0 Nov 13, 2012

Contributor

For the cases mentioned by @Mossop, I will add at least one example to be clear.
Also, I would say that this 18 weeks is the minimum period. I mean, a deprecated module could stay longer in the code base before fully removed, but the developers shouldn't rely on that. Not sure if it's worthy to mention.

@ZER0 ZER0 commented on the diff Nov 13, 2012

doc/dev-guide-source/guides/stability.md
+no longer optimal or supported by the underlying platform.
+
+The API lifecycle aims to balance these competing demands. It has two
+main components:
+
+* a [stability index](dev-guide/guides/stability.html#Stability Index)
+that defines how stable each module is
+* a [deprecation process](dev-guide/guides/stability.html#Deprecation Process)
+that is intended to enable the SDK to remove or change APIs when necessary,
+while giving developers enough time to update their code.
+
+## Stability Index ##
+
+The stability index is adopted from
+[node.js](http://nodejs.org/api/documentation.html#documentation_stability_index).
+The SDK uses only three of the six values defined by node.js:
@ZER0

ZER0 Nov 13, 2012

Contributor

Good to know, I thought we use actually all of them. :)

@ZER0 ZER0 commented on the diff Nov 13, 2012

doc/dev-guide-source/guides/stability.md
+[node.js](http://nodejs.org/api/documentation.html#documentation_stability_index).
+The SDK uses only three of the six values defined by node.js:
+
+<table>
+ <tr>
+ <td>Experimental</td>
+ <td>The module is not yet stabilized.
+You can try it out and provide feedback, but we may change or remove
+it in future versions without having to pass through a formal
+deprecation process.</td>
+ </tr>
+ <tr>
+ <td>Stable</td>
+ <td>The module is a fully-supported part of
+the SDK. We will avoid breaking backwards compatibility unless absolutely
+necessary. If we do have to make backwards-incompatible changes, we will
@ZER0

ZER0 Nov 13, 2012

Contributor

It's definitely fine as is, but maybe it could be good have an example about what we meant with "absolutely necessary" (e.g. Security issues, Platform changes – like the private browing stuff, etc).

@ZER0 ZER0 and 1 other commented on an outdated diff Nov 13, 2012

doc/dev-guide-source/guides/stability.md
+ <td>Deprecated</td>
+ <td>We plan to change this module, and backwards compatibility
+should not be expected. Don’t start using it, and plan to migrate away from
+this module to its replacement.</td>
+ </tr>
+</table>
+
+The stability index for each module is written into that module’s
+metadata structure, and is displayed at the top of each module's
+documentation page.
+
+## Deprecation Process ##
+
+### Preparation ###
+
+Before deprecating a module, the SDK team will develop and document
@ZER0

ZER0 Nov 13, 2012

Contributor

This is seems not necessary true. To me "deprecating" means "mark as deprecated", but we don't necessary have a full alternative ready yet, even if the module has this stability flag. In this sentence it seems more like "before remove a module marked as deprecated". So it's probably depends by my English, but to me it seems vague.

@wbamberg

wbamberg Nov 21, 2012

Contributor

Yes, to me deprecating means "mark as deprecated" as well. I had thought we would have an alternative at the time we deprecate something. Then the purpose of the deprecation period is to give developers time to migrate to the new code. If we don't have anything to offer developers as a migration path, what good is it to deprecate?

I don't really think it's a good story to tell developers to stop using an API and to migrate away from it, without offering them a replacement. But maybe I'm confused about this?

@ZER0

ZER0 Nov 21, 2012

Contributor

I'm thinking about the current state of Add-on SDK: we have a bounch of modules that are marked as deprecated, but we don't have already a full functional alternative for each of them. So, to me we mark as deprecated a module when the module shouldn't be used, because we're going to deprecate it soon or later. Then the situation could be:

  1. We already have a full functional replacement for it (redirect the devs)
  2. We don't have a full functional replacement for it yet (e.g. window/utils compare to window-utils)
  3. We don't support that functionality anymore (e.g. for platform dependency), or it was an experimental low level API (e.g. cortex, traits, etc). We can suggest alternative approaches, but not an equivalent module.

The common denominator is "mark as deprecated" means "be aware that we're not going to support this module anymore, and we're going to remove it as soon as we can, but not earlier than 18 weeks".

Mark a module as deprecated is useful when we have internal dependencies with these modules: so we can't remove it yet, but we want to be sure that the devs are not going to use them – or, if they use them, at least they know very well that are deprecated.
This is so far the most common case we have currently.

@ZER0 ZER0 and 1 other commented on an outdated diff Nov 13, 2012

doc/dev-guide-source/guides/stability.md
+ <td>We plan to change this module, and backwards compatibility
+should not be expected. Don’t start using it, and plan to migrate away from
+this module to its replacement.</td>
+ </tr>
+</table>
+
+The stability index for each module is written into that module’s
+metadata structure, and is displayed at the top of each module's
+documentation page.
+
+## Deprecation Process ##
+
+### Preparation ###
+
+Before deprecating a module, the SDK team will develop and document
+an alternative, and decide which release to deprecate it in.
@ZER0

ZER0 Nov 13, 2012

Contributor

Question: this is always the case? I would leave open the possibility, like we did for the "Stable" API ("avoid breaking backwards compatibility unless absolutely necessary") that we could remove a module without necessary provide a full alternative to it. I see it as a rare condition, but like Stable API we can't really said that it can't be happen (e.g. the platform code change so much that a specific functionality make no sense anymore).

@wbamberg

wbamberg Nov 21, 2012

Contributor

Yeah, that's true enough.

@ZER0 ZER0 and 1 other commented on an outdated diff Nov 13, 2012

doc/dev-guide-source/guides/stability.md
+
+* update the module's stability index to be "deprecated"
+* include a deprecation notice in the
+[release notes](https://wiki.mozilla.org/Labs/Jetpack/Release_Notes),
+the [Add-ons blog](https://blog.mozilla.org/addons/), and the
+[Jetpack Google group](https://groups.google.com/forum/?fromgroups#!forum/mozilla-labs-jetpack). The deprecation notice should point developers at a migration guide.
+
+### Migration ###
+
+The deprecation period defaults to 18 weeks (that is, three releases).
+During this time, the module will be in the deprecated state. The SDK
+team will track usage of deprecated modules on
+[addons.mozilla.org](https://addons.mozilla.org/en-US/firefox/) and support
+developers migrating their code. The SDK will continue to provide warnings:
+
+* CFX will generate warnings when developers use deprecated modules.
@ZER0

ZER0 Nov 13, 2012

Contributor

good to be generic, because at the beginning we'll probably generate warnings / log errors only at runtime (cfx run), and generate them at building time (both cfx run and cfx xpi) only later. Notice that at the moment, none of them are supported. But for the first one, I could actually add the implementation at the same time of bug 725409.

@ZER0

ZER0 Nov 18, 2012

Contributor

So, I can't actually implemented the warning about "deprecated" in bug 725409. The problem is, it will log an error for all usage of any deprecated module, not just the one used in the add-on but also the one used in the SDK module: we still use a lot of deprecated module in our modules.

I don't think we want to bother the user, neither the AMO reviewer, with this list. However, all deprecated modules should be under deprecated folder, so it should be quite easy spot their usage both for developers and AMO reviewer.

We probably implement that on cfx side soon or later, but as far as I know, there is not plan to do so at the moment. So I will suggest to remove this note from the docs, and maybe add that all deprecated modules are under homonyms folder.

@wbamberg

wbamberg Nov 19, 2012

Contributor

Just my opinion, but this seems backwards to me.

If generating warnings when people use deprecated APIs is the best thing to do, that's what we should do. If we get problems because we're using deprecated APIs, we should not use deprecated APIs. If for some reason we have to use deprecated APIs, we should suppress the warnings when they're due to SDK code using deprecated modules.

We should choose a policy based on what's best for our users, not based on what's easiest for us to achieve.

Also, I don't think we should signal deprecation by the modules' position in the tree, a point I've made before: https://bugzilla.mozilla.org/show_bug.cgi?id=793926#c7. If we're using metadata to indicate deprecatedness, it's redundant to use the file location as well. Also, I may be wrong, but if we signal deprecation by file location, then we have to move modules when we deprecate them, and won't we then break anyone who uses them? (unless we hack cfx each time).

@ZER0

ZER0 Nov 21, 2012

Contributor

Maybe I'm wrong, but I thought that one reason because we mark a module as deprecated, is to be sure that even if they're still present the devs are not going to use them – or at least they are perfectly aware they're deprecated.

They can be still present not only because we want to give to the devs the time to migrate the code, but also because internally we could still have some dependencies that wasn't so immediate get rid of.

I believe that we should choose a policy based on what's best for our users, and at the same time based on what's possible for us to achieve in reasonable time and resources: we can focus to refactor the entire code base and modules that uses deprecated APIs, but it's not realistic. We should doing a progressive enhancement, and replace what we can when we work on them, until we wipe them out. In the meantime, we can mark those modules "deprecated" for the users.

I agree with you, we shouldn't signal deprecation by modules' position, I was just point it out that currently the deprecated modules are under this folder, so it's easy spot them out for both devs and reviewers. My point was that we can't really write that sentence, because at the moment CFX is not raise warnings for deprecated module, and it's less trivial than I thought on JS side. I'm trying a couple of approach in bug 725409 but I will probably to end up to land it as is and open another bug for this.
If the "future" tense used in the sentence is clear enough, we can leave it ("CFX will generate warnings" = "CFX currently doesn't generate warnings, but it will in the future"); otherwise I will keep this pull in stand by until we implemented the behavior described.

@wbamberg

wbamberg Nov 21, 2012

Contributor

Yeah, that's fair enough. I'll change this to be more explicit that we won't immediately be able to issue these warnings. If you remember to remind me to update this when we do start issuing warnings, that would be great!

@ZER0 ZER0 commented on the diff Nov 13, 2012

...dlefish/tests/static-files/lib/sdk/aardvark-feeder.js
@@ -0,0 +1,7 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+module.metadata = {
+ "stability": "stable"
+};
@ZER0

ZER0 Nov 13, 2012

Contributor

We should have new line at the end of the file – unless is not specifically required by the tests we're doing. Same for the files above.

wbamberg merged commit da566f7 into mozilla:master Nov 21, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment