Closed Bug 1530868 Opened 2 years ago Closed 2 years ago

Fix module imports in chat code

Categories

(Chat Core :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Instantbird 67

People

(Reporter: clokep, Assigned: clokep)

References

Details

Attachments

(1 file)

Bug 1520643 made a bunch of changes to how we import modules, unfortunately as part of this it seems that all of the possible exported symbols for each module were imported into each other file inside of chat. Oddly there's also a few spots where we're missing the proper imports.

Attached patch Patch v1Splinter Review

The locations to fix were found by using eslint and looking at the no-undef and no-unused-vars errors.

Note that it seems there were some place where modules were imported that were never used at all!

Attachment #9046885 - Flags: review?(martin)
Comment on attachment 9046885 [details] [diff] [review]
Patch v1

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

I'm assuming the additions/removals work, given that I don't currently have a build setup. They all look sensible and assuming all the paths acutall work (I couldn't find any obvious typos) and eslint doesn't complain this should be fine.

::: chat/protocols/irc/ircCTCP.jsm
@@ +14,2 @@
>  const {ircHandlers} = ChromeUtils.import("resource:///modules/ircHandlers.jsm");
> +var {_} = ChromeUtils.import("resource:///modules/ircUtils.jsm");

It's a bit weird to have one import that uses var and the other two using const, though var seems to be the more prevalent one? I think the different styles come from multiple code transformations and when the code was written. Is the chosen style to always use var for new ones, and existing const ones are not touched?

::: chat/protocols/matrix/matrix-sdk.jsm
@@ -7,2 @@
>  const {clearInterval, clearTimeout, setInterval, setTimeout} = ChromeUtils.import("resource://gre/modules/Timer.jsm");
> -Cu.importGlobalProperties(["XMLHttpRequest"]);

Huh, is XHR now just available in JSMs?
Attachment #9046885 - Flags: review?(martin) → review+
Blocks: 1520643

(In reply to Martin Giger [:freaktechnik] from comment #2)

::: chat/protocols/irc/ircCTCP.jsm
@@ +14,2 @@

const {ircHandlers} = ChromeUtils.import("resource:///modules/ircHandlers.jsm");
+var {_} = ChromeUtils.import("resource:///modules/ircUtils.jsm");

It's a bit weird to have one import that uses var and the other two using
const, though var seems to be the more prevalent one? I think the different
styles come from multiple code transformations and when the code was
written. Is the chosen style to always use var for new ones, and existing
const ones are not touched?

I also find this super weird that it is sometimes var and sometimes const. florian and I discussed this on IRC and it seems they should each be const, but it doesn't really matter. I'm going to ignore this for now.

::: chat/protocols/matrix/matrix-sdk.jsm
@@ -7,2 @@

const {clearInterval, clearTimeout, setInterval, setTimeout} = ChromeUtils.import("resource://gre/modules/Timer.jsm");
-Cu.importGlobalProperties(["XMLHttpRequest"]);

Huh, is XHR now just available in JSMs?

That's what eslint told me. This code needs to be re-worked anyway, so I don't think this is a big deal.

Pushed by clokep@gmail.com:
https://hg.mozilla.org/comm-central/rev/5b2feb6ef349
Fix imports in chat after bug 1520643. r=freaktechnik

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 67

Re var vs const: yes in modules the preferred way is really const.
We had to change a bunch of non-module js files to use var since otherwise there would be failures due to re-assignment...

You need to log in before you can comment on or make changes to this bug.