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.

Sign up for GitHub

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

Jump to bottom

Copy getters and setters correctly in interopWildcard #6850

Merged
merged 3 commits into from
Nov 30, 2017
Merged

Copy getters and setters correctly in interopWildcard #6850

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7c2bbb4
Copy getters and setters correctly in interopWildcard
danez Nov 18, 2017
25fb92b
Add another test for star import
danez Nov 18, 2017
7a9f39d
Add check for defineProperty and getOwnPropertyDescriptor
danez Nov 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion packages/babel-helpers/src/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,14 @@ helpers.interopRequireWildcard = defineHelper(`
var newObj = {};
if (obj != null) {
for (var key in obj) {
if (Object.prototype.hasOwnProperty.call(obj, key)) newObj[key] = obj[key];
if (Object.prototype.hasOwnProperty.call(obj, key)) {
var desc = Object.getOwnPropertyDescriptor(obj, key);
if (desc.get || desc.set) {
Object.defineProperty(newObj, key, desc);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we're going to do this, we should check for the existence of Object.defineProperty, or we should finally expose a flag for modules to opt in or out of Object.defineProperty usage. Otherwise this is going to make things unusable for people targeting old environments.

Copy link
Member Author

@danez danez Nov 18, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a check for Object.defineProperty, which I think should be enough here, because when Object.defineProperty is not there then getters and setters are also not supported.

It is basically supported in all environments besides IE8 and older. But IE8 is also over 10years old now. 🕵️‍♀️
What if we change in babel 7 and state that generated code from babel needs at least ES5, like react does now (they even require Map and Set). Would be nice to have statistics to see what environments people support, but I guess that does not really exist. Or did we have already complains about this?

Copy link
Member Author

@danez danez Nov 18, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And core-js also has a polyfill for it, so we could also link people there. And from my experience if you support IE8 or older, then you need polyfills anyway.

} else {
newObj[key] = obj[key];
}
}
}
}
newObj.default = obj;
Expand Down
3 changes: 3 additions & 0 deletions ...-plugin-transform-modules-commonjs/test/fixtures/misc/copy-getters-setters-star/actual.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import * as foo from "./moduleWithGetter";

export { foo };
5 changes: 5 additions & 0 deletions ...el-plugin-transform-modules-commonjs/test/fixtures/misc/copy-getters-setters-star/exec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import * as foo from "./moduleWithGetter";

assert.throws(() => foo.boo);

// No exception should be thrown
12 changes: 12 additions & 0 deletions ...lugin-transform-modules-commonjs/test/fixtures/misc/copy-getters-setters-star/expected.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
"use strict";

Object.defineProperty(exports, "__esModule", {
value: true
});
exports.foo = void 0;

var foo = _interopRequireWildcard(require("./moduleWithGetter"));

exports.foo = foo;

function _interopRequireWildcard(obj) { if (obj && obj.__esModule) { return obj; } else { var newObj = {}; if (obj != null) { for (var key in obj) { if (Object.prototype.hasOwnProperty.call(obj, key)) { var desc = Object.getOwnPropertyDescriptor(obj, key); if (desc.get || desc.set) { Object.defineProperty(newObj, key, desc); } else { newObj[key] = obj[key]; } } } } newObj.default = obj; return newObj; } }
6 changes: 6 additions & 0 deletions ...ansform-modules-commonjs/test/fixtures/misc/copy-getters-setters-star/moduleWithGetter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
var Obj = {
baz: 123,
get boo() { throw new Error('Should never be triggered'); }
}

module.exports = Obj;
3 changes: 3 additions & 0 deletions ...babel-plugin-transform-modules-commonjs/test/fixtures/misc/copy-getters-setters/actual.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import Foo, { baz } from "./moduleWithGetter";

export { Foo, baz };
3 changes: 3 additions & 0 deletions ...s/babel-plugin-transform-modules-commonjs/test/fixtures/misc/copy-getters-setters/exec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import Foo, { baz } from "./moduleWithGetter";

// No exception should be thrown
21 changes: 21 additions & 0 deletions ...bel-plugin-transform-modules-commonjs/test/fixtures/misc/copy-getters-setters/expected.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
"use strict";

Object.defineProperty(exports, "__esModule", {
value: true
});
Object.defineProperty(exports, "Foo", {
enumerable: true,
get: function () {
return _moduleWithGetter.default;
}
});
Object.defineProperty(exports, "baz", {
enumerable: true,
get: function () {
return _moduleWithGetter.baz;
}
});

var _moduleWithGetter = _interopRequireWildcard(require("./moduleWithGetter"));

function _interopRequireWildcard(obj) { if (obj && obj.__esModule) { return obj; } else { var newObj = {}; if (obj != null) { for (var key in obj) { if (Object.prototype.hasOwnProperty.call(obj, key)) { var desc = Object.getOwnPropertyDescriptor(obj, key); if (desc.get || desc.set) { Object.defineProperty(newObj, key, desc); } else { newObj[key] = obj[key]; } } } } newObj.default = obj; return newObj; } }
6 changes: 6 additions & 0 deletions ...in-transform-modules-commonjs/test/fixtures/misc/copy-getters-setters/moduleWithGetter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
var Obj = {
baz: 123,
get boo() { throw new Error('Should never be triggered'); }
}

module.exports = Obj;
2 changes: 1 addition & 1 deletion ...babel-plugin-transform-modules-commonjs/test/fixtures/misc/import-const-throw/expected.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ var Bar = _interopRequireWildcard(require("bar"));

var _baz = require("baz");

function _interopRequireWildcard(obj) { if (obj && obj.__esModule) { return obj; } else { var newObj = {}; if (obj != null) { for (var key in obj) { if (Object.prototype.hasOwnProperty.call(obj, key)) newObj[key] = obj[key]; } } newObj.default = obj; return newObj; } }
function _interopRequireWildcard(obj) { if (obj && obj.__esModule) { return obj; } else { var newObj = {}; if (obj != null) { for (var key in obj) { if (Object.prototype.hasOwnProperty.call(obj, key)) { var desc = Object.getOwnPropertyDescriptor(obj, key); if (desc.get || desc.set) { Object.defineProperty(newObj, key, desc); } else { newObj[key] = obj[key]; } } } } newObj.default = obj; return newObj; } }

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

Expand Down