further restrict property descriptor type definitions

[michaelficarra]

I was getting an error on this code due to a loose definition of property descriptors:

function safeWrite(obj: {}, prop: string, value: any) {
  let desc = Object.getOwnPropertyDescriptor(obj, prop);
  if (desc && 'value' in desc && (desc.writable || desc.configurable)) {
    desc.value = value;
    Object.defineProperty(obj, prop, desc);
  }
}

The error message was

Sketchy null check on boolean [1] which is potentially false. Perhaps you meant to check for null or undefined [1]?
(sketchy-null-bool)

     xxx.js
      8│
      9│ function safeWrite(obj: {}, prop: string, value: any) {
     10│   let desc = Object.getOwnPropertyDescriptor(obj, prop);
     11│   if (desc && 'value' in desc && (desc.writable || desc.configurable)) {
     12│     desc.value = value;
     13│     Object.defineProperty(obj, prop, desc);
     14│   }

     /tmp/flow/flowlib_16ab40eb/core.js
 [1] 27│     writable?: boolean;

[michaelficarra]

It seems tests are failing because I haven't updated error offsets. How do I go about doing that? I can't find development documentation.

[mrkev]

Hmm, let me bring this in and check it out. I can update the tests nw 👍

[facebook-github-bot]

@mrkev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[mrkev]

Could you provide some documentation that backs up the changes you made here?

[michaelficarra]

@mrkev Are you asking for me to add documentation to the PR or are you asking for me to provide you links to the sections of the spec that define the affected functions?

[mrkev]

The latter one 👍 in a comment here or the description of the PR is good.

[michaelficarra]

For more information about the Property Descriptor specification type, see https://tc39.github.io/ecma262/#sec-property-descriptor-specification-type.

ContraPropertyDescriptor represents the property descriptor-like objects accepted by APIs which manipulate property descriptors. These objects are converted to proper property descriptors by ToPropertyDescriptor.

CoPropertyDescriptor represents objects returned by APIs which read property descriptors. These objects are more restricted as each of the property descriptor's properties will always be present.

[michaelficarra]

@mrkev I've provided the information that you asked for here. Can we get this merged?

[michaelficarra]

@mrkev It's been another few months and I've yet again been having difficulties with Flow due to its loose typing of property descriptors. Is there anything else I can do to get this PR merged? I believe we just need to update error offsets, but there's no documentation on how to do that.

[michaelficarra]

@mrkev I run into this issue nearly every time I write code that uses Flow. The implementation work here has already been done. Is there anything I can do to get somebody to merge this? I would think the Flow team would be more concerned about incorrect descriptions of JavaScript standard library APIs.

[bakkot]

Ping @\maintainers. It's been another ~6 months.

[goodmind]

@mrkev

[mrkev]

@goodmind @thread been swamped with FB5 work. Probably won't be able to look at it for another while, sorry /:

[michaelficarra]

@mrkev is there a different maintainer that can look at it? It's not a very complicated change.

[mrkev]

Hop into the discord channel and ask there 👍

[goodmind]

/cc @nmote

[bakkot]

Ping @nmote etc. It's been another ~6 months.