Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The function exported using Object.defineProperty in node_modules cannot be found. #56304

Open
shlroland opened this issue Dec 18, 2024 · 7 comments

Comments

@shlroland
Copy link

Version

v22.9.0

Platform

Darwin mini.local 24.0.0 Darwin Kernel Version 24.0.0: Tue Sep 24 23:36:26 PDT 2024; root:xnu-11215.1.12~1/RELEASE_ARM64_T8103 arm64

Subsystem

No response

What steps will reproduce the bug?

related issue #56286

reproduction code demo_import.zip

Advance declaration: The code inside the current node_modules which is in reproduction code, I did not use any package manager(npm/yarn/pnpm) for installation, but rather manually copied and pasted it in, ensuring the exclusion of interference from package management.

Steps:
Download and unzip the reproduction code, and directly execute node index.js.

This is my execution result

Image

How often does it reproduce? Is there a required condition?

100%

What is the expected behavior? Why is that the expected behavior?

shouldImport This function can be directly accessed in the first layer of the module object, rather than only being obtainable in the internal default object.

What do you see instead?

shouldImport was lost during the conversion from cjs to esm

Additional information

I also tried to execute the code using bun(1.1.40), and the result was correct.

Image

@shlroland
Copy link
Author

Some even stranger phenomena have been observed.

Kapture.2024-12-18.at.20.53.19.mp4

@aduh95
Copy link
Contributor

aduh95 commented Dec 18, 2024

Can you share a repro that doesn’t involve running code downloaded from the internet? Ideally a short list of commands that can be run from a CLI, with the expected output and an explanation for why the actual output is not acceptable

@shlroland
Copy link
Author

shlroland commented Dec 19, 2024

Can you share a repro that doesn’t involve running code downloaded from the internet? Ideally a short list of commands that can be run from a CLI, with the expected output and an explanation for why the actual output is not acceptable

You should create a new folder to perform the following steps.

# 1. create the necessary folders
rm -rf repro
mkdir -p repro/node_modules/should_import
cd repro

# 2. generating a simulated cjs package 
cat -<<'EOF' > node_modules/should_import/methods.js
'use strict';
Object.defineProperty(exports, '__esModule', { value: true });
var shouldImport = function (a, b) { return a + b; };
exports.default = shouldImport;
EOF

cat -<<'EOF' > node_modules/should_import/util.js
'use strict';
Object.defineProperty(exports, '__esModule', { value: true });
exports.util = void 0;
var util = function () { return "util"; };
exports.util = util;
EOF

cat -<<'EOF' > node_modules/should_import/index.js
"use strict";
var __importDefault = (this && this.__importDefault) || function (mod) {
    return (mod && mod.__esModule) ? mod : { "default": mod };
};
Object.defineProperty(exports, "__esModule", { value: true });
exports.util = exports.shouldImport = void 0;
var methods_1 = require("./methods");
Object.defineProperty(exports, "shouldImport", { enumerable: true, get: function () { return __importDefault(methods_1).default } });
exports.shouldImport_1 = methods_1.default;

var util_1 = require("./util");
Object.defineProperty(exports, "util", { enumerable: true, get: function () { return util_1.util; } });
exports.util_1 = util_1.util;
EOF

# 3. generating a simulated demo project
cat -<<'EOF' > index.mjs                                   
import * as ShouldImport from "should_import";

console.log('Does "should_import" export a "shouldImport" named export:', 'shouldImport' in ShouldImport);
EOF

# 4. run the code
node index.mjs

# 5. Cleanup
cd ..
rm -rf repro

As you can observe, it outputs Does "should_import" export a "shouldImport" named export: false.

this is the unacceptable output: shouldImport This function can not be directly accessed in the first layer of the module object, rather than only being obtainable in the internal default object.

Let's make some modifications.

// node_modules/should_import/index.js 
// Remove the wrapper of the __importDefault function.
// Object.defineProperty(exports, "shouldImport", { enumerable: true, get: function () { return __importDefault(methods_1).default } });

Object.defineProperty(exports, "shouldImport", { enumerable: true, get: function () { return methods_1.default } });

It now outputs Does "should_import" export a "shouldImport" named export: true.

this is the expected output: shouldImport can be directly accessed in the first layer of the module object

Especially important

Just like the video in my second comment, removing the __importDefault wrapper and keeping only the parentheses will also yield unacceptable results.

Additional information

Using bun instead of node for execution, both times can yield the expected result.

@aduh95
Copy link
Contributor

aduh95 commented Dec 19, 2024

Thanks for the repro, I took the liberty of removing the screenshots which can be harder to read (e.g. folks using screen readers may only have access to the alt text).

I suspect this is a known limitation of nodejs/cjs-module-lexer, which is frozen as explained in the project status – although there's an open issue to reconsider that status: nodejs/cjs-module-lexer#101

@shlroland
Copy link
Author

Having thoroughly reviewed the README of nodejs/cjs-module-lexer, I suspect that this problem might be connected to the dynamic function.

If the code is adjusted to the following form, that is, moving __importDefault(methods_1).default outside the Object.defineProperty function call, it will take effect.

const shouldImport = __importDefault(methods_1).default 
Object.defineProperty(exports, "shouldImport", { enumerable: true, get: function () { return shouldImport } });

Additionally, the original code here was compiled using tsc. If esModuleInterop is enabled in tsconfig.json, it will generate the __importDefault function to achieve compatibility with the cjs format for the import default syntax in esm.

So there should also be an issue opened in the TypeScript repository.

@aduh95
Copy link
Contributor

aduh95 commented Dec 19, 2024

Yeah, if we can get the transpilation output to not call a function in the getter, that's much better for sure – and would probably better fit the spec. The way the ECMAScript spec is written, we cannot have side-effects of accessing a named export, and it seems like a very hard problem to detect whether calling a function such as __importDefault would have side-effects – I wonder how Bun does it, maybe they have a special case for the TS output 🤔

/cc @nodejs/loaders

@shlroland
Copy link
Author

I searched in the typescript repository using cjs-module-lexer as a keyword, and indeed found similar issues.

Considering the current general situation of cjs and ts, it may only be possible to call on the package authors to try to update and provide native esm for node to call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants