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

RESOLVED FIXED in 6.2

Status

defect
P1
normal
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: Taraman, Assigned: Fallen)

Tracking

({meta})

Dependency tree / graph

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

6 years ago
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.
Reporter

Updated

6 years ago
Depends on: 905101
Reporter

Updated

6 years ago
Depends on: 906107
Reporter

Updated

6 years ago
No longer depends on: 408207
Depends on: 1346808
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?
Flags: needinfo?(makemyday)
Flags: needinfo?(Mozilla)
Depends on: 1347149
Depends on: 1347211
Depends on: 1347225
Reporter

Comment 2

2 years ago
(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.
Flags: needinfo?(Mozilla)
Priority: -- → P2
No longer depends on: 487207
Depends on: 1408662
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.
Flags: needinfo?(makemyday)
Assignee: Mozilla → philipp
Priority: P2 → P1
Depends on: 1432142
Depends on: 1433156
Depends on: 1433229
Depends on: 1433802
Depends on: 1436488
Depends on: 1436490
Depends on: 1436491
Depends on: 1436557
Depends on: 1436571
Depends on: 1439868
Depends on: 1439900
Depends on: 1440249
Depends on: 1440490
Depends on: 1440587
Depends on: 1440980
Depends on: 1440982
Posted patch FInal Patch - v1 (obsolete) β€” β€” Splinter Review
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.
Attachment #8953887 - Flags: review?(makemyday)
Duplicate of this bug: 1452120

Comment 7

Last year
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.
Attachment #8953887 - Flags: review?(makemyday) → review+
Posted patch Final Patch - v2 β€” β€” Splinter Review
Thanks for catching that!
Attachment #8953887 - Attachment is obsolete: true
Attachment #8967869 - Flags: review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
Attachment #8967869 - Flags: approval-calendar-beta+

Comment 9

Last year
Pushed by mozilla@kewis.ch:
https://hg.mozilla.org/comm-central/rev/1934bf85b477
The final nail in the coffin of calUtils.js. r=MakeMyDay
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED
(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?
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.
You need to log in before you can comment on or make changes to this bug.