905097 - Consolidate calendar Helper functions and switch to calUtils.jsm [meta]

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect, P1)

Description

[Markus Adrario [:Taraman]]

This meta-bug has the purpose to track the Progress of switching from calUtils.js to calUtils.jsm

The task will be split up into several bugs to keep the patches small.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Something we should discuss regarding this bug is how modules should look in the future. I had started to move various functions to separate namespaces, e.g. cal.dtz.startDateProp. There was just one cyclic dependency for this and I believe I found a solution for it. The advantage about this approach is that we have functions in separate files and namespaces and that it is always clear where the functions come from.

The downside is that while it works well with the JSM module loader where objects are shared, I believe we will have to make further changes to switch to ES6 modules. There is |import * as lib from 'lib'| which would import all functions into the "lib" object, but I don't know if |import * as cal.dtz from "/path/to/calDateTimeUtils.jsm"| would work. In any case, we would need to either change the way it is imported and used, or alternatively the way it is exported.

A more ES6-like way with the current loader would be to export the single function names:

this.EXPORTED_SYMBOLS = [ ... , "startDateProp", ... ];

/* export */ function startDateProp(aItem) { ... }

and then use something like this to do the import (where it is used), possibly with a helper function to not use the backstage pass but actually the exported object in the destructuring:

/* For now        */ var { startDateProp } = Components.utils.import("resource://...", {});
/* When using ES6 */ import { startDateProp } from "resource://...";


It would be slightly more complicated if we want to use a namespace. I am not totally sure this works in ES6, and I can't come up with similar syntax for JSMs. I am also not sure if it is possible to import into an existing object with ES6. Here is the closest I can offer:

function calimport(path, symbols) {
  let exported = {};
  Components.utils.import(path, exported);
  let result = {};
  for (let symbol of symbols) {
    result[symbol] = exported[symbol];
  }
}

/* For now        */ var caldtz = calimport("resource://...", ["startDateProp"]);
/* When using ES6 */ import { startDateProp } as caldtz from "resource://...";

What do you think?

Comment 2

[Markus Adrario [:Taraman]]

(In reply to Philipp Kewisch [:Fallen] from comment #1)
> There is |import * as lib from 'lib'| which would import all
> functions into the "lib" object, but I don't know if |import * as cal.dtz
> from "/path/to/calDateTimeUtils.jsm"| would work. 
That's something that we could easily find out by trial.
If not, would 
   import * as some from calSomeUtils.jsm;
   cal.some = some;
work as workaround maybe?

Generally I like the use of namespaces, but as far as I researched, ES6 Modules don't support them since it is not needed.

If we change something now, I think we should get the code as close as possible towards ES6, so the changes when switching are as small as possible.

Going with your suggestion:
>A more ES6-like way with the current loader would be to export the single function names:
>this.EXPORTED_SYMBOLS = [ ... , "startDateProp", ... ];
>/* export */ function startDateProp(aItem) { ... }

>and then use something like this to do the import (where it is used), possibly with a helper function to not use the >backstage pass but actually the exported object in the destructuring:

>/* For now        */ var { startDateProp } = Components.utils.import("resource://...", {});
>/* When using ES6 */ import { startDateProp } from "resource://...";

would take us a good way towards this.

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

The patch in bug 1408662 goes with what I created before I had made above comments, until es6 modules are a thing I'd rather keep the namespacing as we have it. Instead of spending time figuring out a hack that gives us something that will possibly work with es6 modules, I'd rather just make another change by then.

I'm going to file similar bugs for cal.acl, cal.item, cal.view, cal.data, cal.window, cal.category, cal.provider, cal.itip, cal.l10n once we've agreed on the approach for cal.dtz. Now that I have an automatic script to make the changes it shouldn't be too much effort to polish my old bitrotted patches for this.

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: FInal Patch - v1

This should be the closing patch, modulo possible regressions. It:
* fixes a few issues from already pushed patches
* moves the files into resource://calendar/modules/utils/ 
* adds shim files into resource://calendar/modules/ for those modules previously included directly to avoid file not found errors
* adds filename descriptions for all utils modules

I have a few ideas for followup, but this solves what we had planned for now. One idea would be to have the submodules export the functions directly, then use ChromeUtils.defineModuleGetter() which might bring a perf win. I might punt that to when we are able to use es6 modules though.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c1701f969c49bc355c303828e4a09c2cce085148&selectedJob=164247264

Comment 7

[[:MakeMyDay]]

Comment on attachment 8953887 [details] [diff] [review]
FInal Patch - v1

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

Looks good except the cut&paste error, r=me with that fixed.

::: calendar/base/modules/utils/calAlarmUtils.jsm
@@ +11,5 @@
> + * Helpers for manipulating calendar alarms
> + */
> +
> +// NOTE: This module should not be loaded directly, it is available when
> +// including calUtils.jsm under the cal.acl namespace.

You want cal.alarm instead of cal.acl here

::: calendar/base/modules/utils/calAsyncUtils.jsm
@@ +10,4 @@
>   */
>  
> +// NOTE: This module should not be loaded directly, it is available when
> +// including calUtils.jsm under the cal.acl namespace.

cal.async

::: calendar/base/modules/utils/calAuthUtils.jsm
@@ +13,4 @@
>   */
>  
> +// NOTE: This module should not be loaded directly, it is available when including
> +// calUtils.jsm under the cal.acl namespace.

cal.auth

::: calendar/base/modules/utils/calCategoryUtils.jsm
@@ +11,5 @@
> + * Helpers for reading and writing calendar categories
> + */
> +
> +// NOTE: This module should not be loaded directly, it is available when
> +// including calUtils.jsm under the cal.acl namespace.

cal.category

::: calendar/base/modules/utils/calDataUtils.jsm
@@ +10,5 @@
> + * Data structures and algorithms used within the codebase
> + */
> +
> +// NOTE: This module should not be loaded directly, it is available when
> +// including calUtils.jsm under the cal.acl namespace.

cal.data

::: calendar/base/modules/utils/calDateTimeUtils.jsm
@@ +11,5 @@
> + * Date, time and timezone related functions
> + */
> +
> +// NOTE: This module should not be loaded directly, it is available when
> +// including calUtils.jsm under the cal.acl namespace.

cal.dtz

::: calendar/base/modules/utils/calEmailUtils.jsm
@@ +11,5 @@
> + * Functions for processing email addresses and sending email
> + */
> +
> +// NOTE: This module should not be loaded directly, it is available when
> +// including calUtils.jsm under the cal.acl namespace.

cal.email

::: calendar/base/modules/utils/calItemUtils.jsm
@@ +15,5 @@
> + * Calendar item related functions
> + */
> +
> +// NOTE: This module should not be loaded directly, it is available when
> +// including calUtils.jsm under the cal.acl namespace.

cal.item

::: calendar/base/modules/utils/calIteratorUtils.jsm
@@ +12,5 @@
> + * Iterators for various data structures
> + */
> +
> +// NOTE: This module should not be loaded directly, it is available when
> +// including calUtils.jsm under the cal.acl namespace.

cal.iterate

::: calendar/base/modules/utils/calItipUtils.jsm
@@ +13,5 @@
>   * Scheduling and iTIP helper code
>   */
> +
> +// NOTE: This module should not be loaded directly, it is available when
> +// including calUtils.jsm under the cal.acl namespace.

cal.itip

::: calendar/base/modules/utils/calL10NUtils.jsm
@@ +8,5 @@
> + * Localization and locale functions
> + */
> +
> +// NOTE: This module should not be loaded directly, it is available when
> +// including calUtils.jsm under the cal.acl namespace.

cal.l10n

::: calendar/base/modules/utils/calPrintUtils.jsm
@@ +11,5 @@
> + * Helpers for printing and print preparation
> + */
> +
> +// NOTE: This module should not be loaded directly, it is available when
> +// including calUtils.jsm under the cal.acl namespace.

cal.print

::: calendar/base/modules/utils/calProviderUtils.jsm
@@ +15,4 @@
>   */
>  
> +// NOTE: This module should not be loaded directly, it is available when
> +// including calUtils.jsm under the cal.acl namespace.

cal.provider

::: calendar/base/modules/utils/calUnifinderUtils.jsm
@@ +11,5 @@
> + * Helpers for the unifinder
> + */
> +
> +// NOTE: This module should not be loaded directly, it is available when
> +// including calUtils.jsm under the cal.acl namespace.

cal.unifinder

::: calendar/base/modules/utils/calViewUtils.jsm
@@ +10,5 @@
> + * View and DOM related helper functions
> + */
> +
> +// NOTE: This module should not be loaded directly, it is available when
> +// including calUtils.jsm under the cal.acl namespace.

cal.view

::: calendar/base/modules/utils/calWindowUtils.jsm
@@ +9,5 @@
> + * Calendar window helpers, e.g. to open our dialogs
> + */
> +
> +// NOTE: This module should not be loaded directly, it is available when
> +// including calUtils.jsm under the cal.acl namespace.

cal.window

::: calendar/base/modules/utils/calXMLUtils.jsm
@@ +12,5 @@
> + * Helper functions for parsing and serializing XML
> + */
> +
> +// NOTE: This module should not be loaded directly, it is available when
> +// including calUtils.jsm under the cal.acl namespace.

cal.xml

::: calendar/base/src/calUtils.js
@@ -3,5 @@
> - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> -
> -/* Goodbye, calUtils.js. You've been a great friend over the years. We'll miss
> - * you and wish you all the best for your departure in one of the next
> - * changesets. */

I was thinking about to keep it like the other shim files and print a deprecation notice if included, since addon may have included it directly. Otoh, it shouldn't have been used that way directly for years, so let's get rid of it.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Final Patch - v2

Thanks for catching that!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1934bf85b477
The final nail in the coffin of calUtils.js. r=MakeMyDay

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

https://hg.mozilla.org/releases/comm-beta/rev/52d3403860a3

Comment 11

[Worcester12345]

(In reply to Philipp Kewisch [:Fallen]  from comment #10)
> https://hg.mozilla.org/releases/comm-beta/rev/52d3403860a3

Is there any sort of date for release of this version?

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

You can find the dates at https://wiki.mozilla.org/RapidRelease/Calendar keeping the correlations from https://developer.mozilla.org/en-US/docs/Mozilla/Calendar/Calendar_Versions in mind. It should be May 9th.