avajs / babel

Babel provider for AVA.
11 stars 3 forks source link

Can not require cjs files inside tests (undefined extension handler for cjs in require-precompiled) #22

Closed gloryofrobots closed 4 years ago

gloryofrobots commented 4 years ago

Please provide details about:

// and cjs.cjs is module.exports = { data:"cjs" };

* What happened
It works fine while running as nodejs script, but when running as an ava test it fails inside
require-precompiled module at the line 18
```oldExtension(module, filename);```
because oldExtension is ```undefined```

To illustrate what`s going on I will post code from require-precompiled and comment on it

function install(precompile, ext, extensions) { //if ext==".cjs" then ext = ext || '.js'; extensions = extensions || require.extensions; var oldExtension = extensions[ext];

    // require.extensions does not have .cjs entry so oldExtension will be undefined
    // very stupid fix would be var oldExtension = extensions[ext] || extensions[".js"];

extensions[ext] = function (module, filename) {
    var source = precompile(filename);
            // this returns null
    if (source) {
        module._compile(source, filename);
        return;
    }
            // and here undefined gets called
    oldExtension(module, filename);
};

}


If you point me to the place where require-precompiled is initialised I can make a PR with fix.
But honestly, I can not find it in the source code.
Or I can make a simple but not very correct fix like `var oldExtension = extensions[ext] || extensions[".js"];`

* What you expected to happen
I expect to be able to require("./somefile.cjs") inside ava test

Minimal test project can be found here https://github.com/gloryofrobots/ava-cjs-require-test
to reproduce `npm run test`

ava version is 3.1.0
ava/babel version is 1.0.0
node version is 13.7
novemberborn commented 4 years ago

@gloryofrobots you were looking in the wrong repository 😄 I've moved the issue.

require-precompiled is installed here: https://github.com/avajs/babel/blob/b2a3676d25f27ae70cbe2bb598b0784392b888b0/index.js#L363

Though cjs.cjs is not a test file, so there isn't anything precompiled to be required. Maybe that's why it's not working right.

gloryofrobots commented 4 years ago

Thanks for pointing out right place to start. The thing is that installPrecompiler breaks global require() function so it does not work with .cjs files in any place not only in tests.

gloryofrobots commented 4 years ago

I do not feel the best way to solve this problem

Let me explain again what is going on

Inside babel/index.js there is this

const precompile = filename => Reflect.has(state.lookup, filename) ? fs.readFileSync(state.lookup[filename], 'utf8') : null;
for (const ext of state.extensions) {
    installPrecompiler(precompile, `.${ext}`);
}

installPrecompiler is called for 2 extensions "js" and "cjs" code inside installPrecompiler expects to have require handler for specified extension inside require.extensions However for ".cjs" there is no specified entry. Node probably does not use this data structure internally or has some fallback mechanism for ".cjs" files.

Let`s look inside installPrecompiler

function install(precompile, ext, extensions) {
       ext = ext || '.js';
    extensions = extensions || require.extensions;
         // if there is no extension handler in extensions[ext] 
        //  oldExtension will be undefined!!!!!
    var oldExtension = extensions[ext];

        extensions[ext] = function (module, filename) {
        var source = precompile(filename);
                // this returns null
        if (source) {
            module._compile(source, filename);
            return;
        }
                // and here undefined gets called as a function
                // ERROR!!
        oldExtension(module, filename);
    };
}

I think it can be fixed in one of two places

We can add to babel/index.js

for (const ext of state.extensions) {
        if(!require.extensions[ext]) {
            require.extensions[ext] = require.extensions[".js"]; 
        }
    installPrecompiler(precompile, `.${ext}`);
}

Or we can change require-precompiled

function install(precompile, ext, extensions) {
       ext = ext || '.js';
    extensions = extensions || require.extensions;
    var oldExtension = extensions[ext] || require.extensions[".js"];

I believe the second variant is much better.

Please tell me what you think is better or maybe you have some other thoughts on the matter

novemberborn commented 4 years ago

Or we can change require-precompiled

function install(precompile, ext, extensions) {
       ext = ext || '.js';
  extensions = extensions || require.extensions;
  var oldExtension = extensions[ext] || require.extensions[".js"];

I believe the second variant is much better.

Please tell me what you think is better or maybe you have some other thoughts on the matter

require-precompiled is under our control so I'm happy to solve it there.

gloryofrobots commented 4 years ago

I made a PR in require-precompiled repository to fix this issue https://github.com/avajs/require-precompiled/pull/2

novemberborn commented 4 years ago

Thanks. It'll probably take me until the weekend before I have time to look into this.

novemberborn commented 4 years ago

This is out as 1.0.1.