Extending exports object detections

[guybedford]

Object expressons like:

    module.exports = {
      render: _render2['default'],
      shallow: _shallow2['default'],
      mount: _mount2['default'],
      ShallowWrapper: _ShallowWrapper2['default'],
      ReactWrapper: _ReactWrapper2['default'],
      configure: _configuration.merge,
      EnzymeAdapter: _EnzymeAdapter2['default']
    };

would still be beneficial to fully support reading their properties.

The challenge is doing this without a full parser that can't handle eg object expression / brace ambiguity details.

One idea is that it could be possible to use a , parser with no knowledge of object assign / expression ambiguity but purely based on matching the first , that happens at the same depth of brace tracking (including all brace types) which might be a way to dodge the harder parsing issues while supporting arbitrary expressions.

Will leave this open as a tracking issue for that kind of support.

[nicolo-ribaudo]

If this gets implemented, we must not forget to also consider template literals in depth tracking:

module.exports = {
  prop1: `${1,notExported}`,
};

[guybedford]

Right, it would effectively be a wrapped and nested implementation of the entire main parser.

[CxRes]

Would it be unreasonable to support this (even if it uses eval) under a flag, for those who want it?

Here is my use case: I am trying to monkey patch nw.js with node builtins (issue: nwjs/nw.js#7639). However, since they are defined as module.exports = {...}, the static lexer cannot decipher named exports. The way node itself handles this, i.e. providing esm versions of builtins written as cjs, is through C routines, a solution not suitable for user land. However if only I could have named exports I can implement this at practically zero cost.

[guybedford]

@CxRes can you share more details on the exact source patterns you need supported? It would be first worth assessing if this feature would really solve the problem.

I am hesitant to add new features at this late stage to this project though as well, since having highly variable compatibility for older versions of Node.js will be quite confusing...

[CxRes]

I am looking to add support for esm version of node built-ins (which are not available on the client/browser side of nwjs). For a pattern example, see the fs file here which is one of the larger and more complex APIs. What I intend to do is have a file with module.exports = require('fs') mapped using import maps to 'fs', so that I can import * from fs in my code.

The alternative is to transpile code to commonjs.

[guybedford]

So this issue wouldn't solve that. That's actually multiple separate issues to get to work:

1. Supporting module.exports = _identifier_ = {... object ...} form
2. Supporting getters / setters on object definition
3. Supporting logical and conditional expressions on object definitions

None of the above are at all related to this issue....

It would have been possible to really dive into these when we were first fleshing this out, but at this late stage in the day I think this is out of scope unfortunately.

You're likely better off just wrapping them in an ESM wrapper.

[CxRes]

I understand! However, given the maintenance burden of wrapping the entire node API (and it is unlikely that nwjs developers are themselves interested in fixing this), it looks like I would have to take the evil route and keep transpiling all my code to cjs (and maintain two versions of the same code).

I find it evil because by sticking to the legacy format, there is less incentive for others, like nwjs authors, to move ahead with esm. We are left in this mess for even longer, despite the herculean effort of good people like you in this space!

[guybedford]

Yes CommonJS will be around for another 10 years at least!

For what it is worth, an ESM wrapper is not that hard to do / not that much maintenance, see eg - https://github.com/jspm/jspm-core/blob/main/src-browser/fs.js.

[CxRes]

For what it is worth, an ESM wrapper is not that hard to do / not that much maintenance,

OMG!!! Maybe not for you, but for lesser mortals that is a bit too much. Not the process of creating such a library (which is straight forward), but the fact that you have to then keep on top of node API and that too different versions of it in perpetuity is borderline madness!!!

[guybedford]

Perhaps you could write a script that does Object.keys(require('fs')) then uses that to emit the wrapper automatically so you could just update it from time to time.

[CxRes]

Thanks!!!! After agonizing over it over the weekend, that is what I ended up implementing!

I was hoping to do this with a virtual file system at application but the I realized to map the paths I would need to start a server in the background, so gave up on that! Extra 25kB it is!

[cwadrupldijjit]

I can say that this specific issue would be wonderful to resolve. The current problem is with this itself.

The reason for it is that jest-runtime uses this package to do code lookup of CommonJS modules in order to help with resolution in the Node VMs that they use to run the tests in.

Unfortunately, when I was attempting to test code that imports (intentionally without mocking) the jsonwebtoken package, it's failing to find the exports assigned to an object being assigned to module.exports.

The code it's trying to parse looks like this:

module.exports = {
  decode: require('./decode'),
  verify: require('./verify'),
  sign: require('./sign'),
  JsonWebTokenError: require('./lib/JsonWebTokenError'),
  NotBeforeError: require('./lib/NotBeforeError'),
  TokenExpiredError: require('./lib/TokenExpiredError'),
};

Looking at the code and why it's not catching the requires that are in the object is because of the openTokenDepth being set to 1 since it's found within the object literal. That just messes with the reexports (if I'm reading that correctly). I imagine there likely needs to be a flag that says if it's the right position inside of an exports object assignment that can still allow for it to catch the requires as well as the object keys.

I can see one other possible limitation to that approach, and that's mostly because of reexports not being caught if they were assigned to a variable before they were added to the exports object, but I'm not sure how in scope that case is. I haven't pored over the code enough to understand everything that's happening, only the stuff that relates to parsing this specific case.

[cwadrupldijjit]

I failed to mention that the only thing it's catching as an export is the decode key. Everything else is ignored.

[cwadrupldijjit]

I think a similar problem was also happening to the mongoose package if you would like to know of another example.