[breaking][do-not-merge] CJS interop changes #13
Conversation
src/index.js
Outdated
| if (ns && ns.__esModule) { | ||
| resolve(ns); | ||
| } else { | ||
| var wrap = Object.create(null); |
There was a problem hiding this comment.
This needs to have a fallback for pre-ES5 environments that lack Object.create(null) (Promises are shimmable; Object.create is not)
There was a problem hiding this comment.
is
function X() {};
X.prototype = null;
new X();correct in older browsers? we just need a nulled proto
There was a problem hiding this comment.
There was a problem hiding this comment.
Right - pre-ES5 and pre-__proto__, it's impossible to have a null prototype object, so it'd need to just be a normal object there.
src/index.js
Outdated
| resolve(ns); | ||
| } else { | ||
| var wrap = Object.create(null); | ||
| wrap.__esModule = true; |
There was a problem hiding this comment.
Is this for babel interop alone? Is __esModule supposed to be enumerable?
There was a problem hiding this comment.
unspecified, transpilers tend not to check enumerability
There was a problem hiding this comment.
It seems like it'd be better to make it non-enumerable then (when Object.defineProperty is available)
src/index.js
Outdated
| var wrap = Object.create(null); | ||
| wrap.__esModule = true; | ||
| wrap.default = ns; | ||
| Object.freeze(wrap); |
There was a problem hiding this comment.
similarly this needs to have a fallback for when Object.freeze is not available.
There was a problem hiding this comment.
if it does not exist I guess we can just not call that fn?
src/index.js
Outdated
| } else { | ||
| var wrap = Object.create(null); | ||
| wrap.__esModule = true; | ||
| wrap.default = ns; |
There was a problem hiding this comment.
similarly, is default supposed to be enumerable?
There was a problem hiding this comment.
There was a problem hiding this comment.
That's for it being an own property - where's it specify the descriptor?
There was a problem hiding this comment.
https://tc39.github.io/ecma262/#sec-module-namespace-exotic-objects-getownproperty-p enumerable is always true
|
@ljharb fixed up, can move the proto to be the |
| enumerable: false, | ||
| configurable: false | ||
| }); | ||
| Object.defineProperty(wrap, 'default', { |
There was a problem hiding this comment.
i think we can assign default unconditionally, since anything with defineProperty will also have freeze, and [[Put]] + freeze results in this descriptor.
|
@ljharb with potential change to |
|
@bmeck Can you clarify what you mean by "potential change to Do we know yet whether |
|
@gdborton we do know that; |
4 participants
Relates to #12
Has breaking change, unclear on intent here so marked as do not merge until discussion.