-
Notifications
You must be signed in to change notification settings - Fork 724
Do not assign named exports into existing default export #3494
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
base: main
Are you sure you want to change the base?
Conversation
308c920
to
1b9be0d
Compare
1b9be0d
to
9ff1aac
Compare
Note that this change is potentially breaking, but I don't think the code that might be broken by it was processed correctly in the first place, in case of existing default export, the old implementation was setting static properties on it, this isn't how esm compat works in node.js/with syntheticDefaultExport works for example. I see that babel might emit this in loose mode: module.exports = MyClass;
module.exports.default = MyClass; But I don't see anything that needs to be done when going backwards |
});`; | ||
|
||
const DEFAULT_EXPORT = `const _default = exports.default ?? exports;`; | ||
const DEFAULT_EXPORT = `let _default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might as well use const and transform into a ternary here
export * from "./globalThis"; | ||
${DEFAULT_EXPORT} | ||
for (var _k in _ns) if (_k !== "default" && _k !== "__esModule" && Object.prototype.hasOwnProperty.call(_ns, _k)) _default[_k] = _ns[_k]; | ||
if (typeof exports === "object" && exports !== null && !("default" in exports)) for (var _k in _ns) if (_k !== "default" && _k !== "__esModule" && Object.prototype.hasOwnProperty.call(_ns, _k)) _default[_k] = _ns[_k]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exports
is always an object
and never null
here. Checking that seems redundant. I'm not sure about the logic that "if there is a default
property present, ignore everything".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exports is always an object and never null here
Why? The code might do
module.exports = null
Or do you mean that it is impossible with reexports?
I haven't encountered this problem in real code, but did add the null check for the other case.
Regarding ignoring everything in presence of default property...
Well, I'm not sure if the other logic is correct right now
module.exports.default = class MyClass {static a = 1;}
module.exports.a = 2;
Results in default export, where a
is rewritten to 2
. (Because pre-this-pr transformation code did _default = MyClass, _default.a = 2
)
It also breaks libraries which have strings and primitives as exports.default
Both of those cases seem to work the same way with node.js as my implementation does, but I'm not sure if I am missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may look at how babel processes its output for reference:
import {default as A, b} from 'c';
console.log(A, b)
// Transpiled to
"use strict";
var _c = _interopRequireWildcard(require("c"));
function _interopRequireWildcard(mod) {
if (mod && mod.__esModule) return mod;
const out = { __proto__: null, default: mod };
if (mod === null || typeof mod !== "object" || typeof mod !== "function") {
return out;
}
for (const exportName in mod) {
if (
exportName !== "default" && mod.hasOwnProperty(exportName) &&
((
desc = Object.getOwnPropertyDescriptor(mod, exportName)
) && (desc.get || desc.set))
) {
Object.defineProperty(out, exportName, desc);
} else {
out[exportName] = mod[exportName];
}
}
return out;
}
console.log(_c.default, _c.b);
import {A, b} from 'c';
console.log(A, b)
// Transpiled to
var _c = require("c");
console.log(_c.A, _c.b);
Note: I have deobfuscated and cleaned _interopRequireWildcard from irrelevant code here, it was looking like this in original (Spoiler)
function _interopRequireWildcard(e, t) { if ("function" == typeof WeakMap) var r = new WeakMap(), n = new WeakMap(); return (_interopRequireWildcard = function (e, t) { if (!t && e && e.__esModule) return e; var o, i, f = { __proto__: null, default: e }; if (null === e || "object" != typeof e && "function" != typeof e) return f; if (o = t ? n : r) { if (o.has(e)) return o.get(e); o.set(e, f); } for (const t in e) "default" !== t && {}.hasOwnProperty.call(e, t) && ((i = (o = Object.defineProperty) && Object.getOwnPropertyDescriptor(e, t)) && (i.get || i.set) ? o(f, t, i) : f[t] = e[t]); return f; })(e, t); }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem that we missing anything, other than getter/setter nonsense (Is it relevant to ESM conversion?) and support for function default exports
TIL: typeof (() => {}) === "function"
I was sure it was object
before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, may the function default export be reason for supabase/postgres failure?
In that issue we do have
error: Uncaught (in promise) TypeError: Class extends value #<Object> is not a constructor or null
Maybe this exported class contains "static default" field?..
Nevermind, the way supabase is transformed it is doing somewhat normal stuff, the other issue seems to be caused by something else:
const node_fetch_1 = __importDefault(require("@supabase/node-fetch"));
const PostgrestError_1 = __importDefault(require("./PostgrestError"));
var __importDefault = (this && this.__importDefault) || function (mod) {
return (mod && mod.__esModule) ? mod : { "default": mod };
};
Object.defineProperty(exports, "__esModule", { value: true });
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
esbuild has better support for cjs. We should do whatever esbuild does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if there is anything to borrow from esbuild, as far as I can see - commonjs transforms in it are performed as a part of bundling process, while commonjs transform in fresh is intended for per-module transformation, and babel is doing well in that regard.
Anyway, the code in this branch seem to behave consistently with node.js esm support, which should be a baseline, as all of those libraries are intended to be working here.
c62542b
to
91fd841
Compare
Fixes: #3492