Re-land typed descriptors and printer #2511
Conversation
This was previously reverted by facebookarchive#2508. Time to recommit.
This wasn't working and was failing Flow.
…tors Flow doesn't support optional fields in classes. This effectively becomes enforced with Babel 7 since it treats the type annotations as field initializers. Which means that these fields always gets created. So I fixed all the callsites that checks for `hasOwnProperty` or `in` that I could find.
There was a problem hiding this comment.
sebmarkbage has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
src/descriptors.js
Outdated
| if (d1.writable !== d2.writable) return false; | ||
| if (d1.enumerable !== d2.enumerable) return false; | ||
| if (d1.configurable !== d2.configurable) return false; | ||
| if (d1.value !== undefined) { |
There was a problem hiding this comment.
There is some asymmetry here, causing equalDescriptors not to be commutative when one value is undefined and the other one isn't. That issue already existed in the original code. I doubt that it's intentional? Maybe it gets rescued by the other invariant that the existance of value and get/set are mutually exclusive. (We could use more such invariants in places.)
Anyway, for clarity, I'd suggest rewriting this case to
if ((d1.value === undefined) !== (d2.value === undefined)) return false;There was a problem hiding this comment.
There are three types of descriptors at the spec level. Generic, Data and Accessors. Lack of value and get/set means it's generic. So if one has a value but the other doesn't, it means that they're not even in the same category of descriptors.
Undefined value doesn't mean empty btw. That's an empty value.
This helper function, for reasons that preceded me, doesn't compare whether the values themselves are equivalent.
Basically, this function is only meant to determine if this needs to use joined Descriptor or if it's enough to use a joined Value.
In theory, accessors doesn't need to use a joined Descriptor neither. We can just use a joined value of the getter and a joined value of the setter.
…ould not itterate through them before
|
I believe I found the source of the issue. It was to do with the |
There was a problem hiding this comment.
trueadm is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| @@ -1003,8 +1006,8 @@ export class PropertiesImplementation { | |||
| // Preserve the existing values of the converted property's [[Configurable]] and [[Enumerable]] attributes and set the rest of the property's attributes to their default values. | |||
| if (O !== undefined) { | |||
| invariant(P !== undefined); | |||
| delete current.writable; | |||
| delete current.value; | |||
| current.writable = undefined; | |||
There was a problem hiding this comment.
Nice catch! I didn't think to search for delete.
Release notes: landing two previously reverted PRs
I found the issue.
Flow doesn't support optional fields in classes. This effectively becomes enforced with Babel 7 since it treats the type annotations as field initializers. Which means that these fields always gets created.
We happened to only use this newer plugin for class fields internally which broke our builds only there.
2b4546d Use
undefinedinstead of missing to represent absent field in descriptors. Fixed all the callsites that checks forhasOwnPropertyorinthat I could find.922d40c Fixes a Flow issue in the text printer.
I filed a follow up issue for ObjectValue since it has the same problem.