1792979 - ./mach esmify unable to migrate JSMs which export a symbol from this

Status: RESOLVED INVALID

(Core :: General, defect)

Description

[Julian Descottes [:jdescottes]]

Take for instance the following file: https://searchfox.org/mozilla-central/rev/f36f074acc09f2dab0e9ffaba34b5f6714dec81d/remote/marionette/action.js

./mach esmify fails to export it because it fails to find the exported symbol action.

So basically it stumbles on

const EXPORTED_SYMBOLS = ["action"];

// ...

this.action = {};

// ...

Comment 1

[Mark Banner (:standard8)]

We have an ESLint rule for this (added in bug 1607331), but it only applies to files with an .jsm extension - since they are the only ones automation can be sure are modules.

It looks like there was bug 1773178 originally to rename the files in remote/ but that was abandoned due to referenced-out of tree concerns (not sure if that was right or not).

Comment 2

[Julian Descottes [:jdescottes]]

(In reply to Mark Banner (:standard8) from comment #1)

We have an ESLint rule for this (added in bug 1607331), but it only applies to files with an .jsm extension - since they are the only ones automation can be sure are modules.

Ah I see, so this issue will only occur with *.js files. In that case not sure it's worth keeping an action item here.
I guess something else which would have helped would simply to have a prompt at the end of the script to list all the files which error-ed, and force the developer to notice them. Otherwise the information can get a bit lost in the logs.

It looks like there was bug 1773178 originally to rename the files in remote/ but that was abandoned due to referenced-out of tree concerns (not sure if that was right or not).

Bug 1773178's scope was bigger than remote/, and I think the out-of-tree concern was not for remote files (that's also what we mentioned in Bug 1773603). Also it seems the shim is now handling -> sys.mjs correctly.

Comment 3

[Kris Maglione [:kmag]]

I'm not really sure what component this belongs in, but build metadata says esmify belongs to Core :: General, so...

Comment 4

[Kris Maglione [:kmag]]

Note that you actually shouldn't be able to define an export via this in an ES module. There's no top-level scope object, and this should be null at the top level.

Comment 5

[Tooru Fujisawa [:arai]]

(In reply to Julian Descottes [:jdescottes] (PTO - back Oct 10) from comment #2)

Ah I see, so this issue will only occur with *.js files. In that case not sure it's worth keeping an action item here.

Yes, those files are overlooked when I convert the global this usage with regular variables (var, let, const, etc), and those files need some fix (explained in the document's "No per-JSM global this object" section) before applying the esmify command.

I guess something else which would have helped would simply to have a prompt at the end of the script to list all the files which error-ed, and force the developer to notice them. Otherwise the information can get a bit lost in the logs.

Sounds good.
Filed bug 1793706.

It looks like there was bug 1773178 originally to rename the files in remote/ but that was abandoned due to referenced-out of tree concerns (not sure if that was right or not).

Bug 1773178's scope was bigger than remote/, and I think the out-of-tree concern was not for remote files (that's also what we mentioned in Bug 1773603). Also it seems the shim is now handling -> sys.mjs correctly.

Yes, the shim handles "js -> sys.mjs", but not "js -> jsm", so the rename was skipped,
and that resulted in overlooking some global this usage...

Comment 6

[Julian Descottes [:jdescottes]]

Thanks Arai! Closing this as invalid, bug 1793706 should be enough here.