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

Copy getters and setters correctly in interopWildcard #6850

Merged
merged 3 commits into from Nov 30, 2017

Conversation

danez
Copy link
Member

@danez danez commented Nov 18, 2017

Q                       A
Fixed Issues? Fixes #4290 Fixes #5790
Patch: Bug Fix? y
Major: Breaking Change? n?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT

This ensures that getters and setters are copied correctly in the interopRequireWildcard helper.

@danez danez added 7.x: regression area: helpers area: react PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Nov 18, 2017
Copy link
Member

@Andarist Andarist left a comment

Choose a reason for hiding this comment

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

maybe we should add an additional loose helper without that logic later?

@nicolo-ribaudo
Copy link
Member

Does this also influence import *? If yes, can you add a test for it?

e.g.

import * as foo from "./foo";
assert.throws(() => foo.bar);
// foo.js
module.exports = {
  get bar() { throw {} }
};

@danez
Copy link
Member Author

danez commented Nov 18, 2017

Good idea, I added it.

@babel-bot
Copy link
Collaborator

babel-bot commented Nov 18, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/5915/

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

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

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.

@existentialism existentialism merged commit 9d9710c into master Nov 30, 2017
@loganfsmyth loganfsmyth deleted the danez-patch-1 branch November 30, 2017 21:55
@julienw julienw mentioned this pull request Feb 13, 2018
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
7.x: regression area: helpers area: react outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Importing crypto from babel throws deprecations warnings because of interopRequireWildcard (T7354)
7 participants