standard-things / esm

Tomorrow's ECMAScript modules today!
Other
5.26k stars 146 forks source link

Possible bug in exported proxy when esm is used in Atom package #872

Open bobjunga opened 4 years ago

bobjunga commented 4 years ago

In Atom packages I use a quasi-singleton pattern of returning a new instance of my plugin class. The plugin class has an 'activate()' method that atom invokes after loading the package.

(1) module.exports = new TreeToolBarAtomPlugin()
(2) export default new TreeToolBarAtomPlugin()

Potential Bug -- exported proxy via (2) does not reveal activate method

After adding esm to my plugin package (using the typical index.js loads main.js pattern), I find that leaving the export line (1), the plugin works but if I update it to (2), it stops working because when the atom code tests 'typeof mainModule.activate' its undefined so it does not attempt to invoke it.

I observe that in both cases the 'mainModule=required(mypkgpath)' reference that atom gets from requiring my package is a proxy but when I use (1) the proxy reveals the activate function to 'typeof mainModule.activate' but with (2) it does not.

When (1) is used, my plugin instance is in proxy.[[Target]].[[Target]] CJSProxy

When (2) is used, my plugin instance is in proxy.[[Target]].default ESMProxy

Question 1: Is this a bug, or should esm exports really be different in some way that I do not understand? I thought that I could use esm inside any package and the result would work if the package is required as a cjs or imported as a es module. Am I mistaken on that?

Question 2: Are export proxies specific to esm, es modules, or all (including cjs require) export/import patterns? Asked another way, do all JS module patterns use proxies for the exported items?

Thanks, for taking the time to consider this.

--BobG

bobjunga commented 4 years ago

Another anomaly

When I use line (1) (old way) to export my instance, the 'this' pointer I see in the constructor is not the one that I see via the proxy even though my constructor is called only once.

I tried using a different singleton pattern by recording my plugin's 'this' pointer in the global 'window' object in its constructor.

class TreeToolBarAtomPlugin {
  constructor() {
    this.view = null;
    window.treeToolbar = this;
  }
  activate() {
    this.view = new MyView();
  }
}
(1) module.exports = new TreeToolBarAtomPlugin() 
//(2) export default new TreeToolBarAtomPlugin()

Then in the console...

console.log(treeToolbar) 
-> Object ...
console.log(treeToolbar.view)   
-> null
console.log(atom.packages.activePackages["bg-tree-view-toolbar"].mainModule.view)   
-> Object ....

If I use line (2) (ems way) to export my instance, the this pointer is consistent. Since in this case, Atom did not activate my package, I activated it manual in the console by referencing it as [proxy].default.activate(). Then, referencing through the global window.treeToolbar and the [proxy].default both shows the same object for this.view (of the plugin's this).

--BobG

bobjunga commented 4 years ago

Writing this issue helped me realize that maybe this is about supporting default export and named exports at the same time? Maybe the 'default' member of the proxy is making room for named exports to exist along side but maybe that is new to es modules and the cjs code does not recognize it?

I modified my index.js by making the pass through assignment promote the 'default' property up to be the entire export like this...

// Set options as a parameter, environment variable, or rc file.
// eslint-disable-next-line no-global-assign
require = require("esm")(module/* , options */)
module.exports = require("./lib/bg-tree-view-toolbar").default

This seems to fix everything. Atom is happy. The global singleton reference works (i.e. the this pointer is not split as stated above).

Possible Bug

I wonder if a module only uses a default export, and there are no named exports if esm was in error to put it in the 'default' property?

--BobG

jsg2021 commented 4 years ago

Not a bug. This is how esm / commonjs interop works.