Fix exception on older browsers

[justinbhopper]

Initial checklist

• I read the support docs
• I read the contributing guide
• I agree to follow the code of conduct
• I searched issues and couldn’t find anything (or linked relevant results below)
• If applicable, I’ve added docs and tests

Description of changes

This addresses errors caused by the unified constructor in ES5 and fixes #245.

[wooorm]

Can you add a test?

[justinbhopper]

Can you add a test?

Sure thing -- I've added tests specifically for CallableInstance that should cover this.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (b69689b) to head (dbc4e74).
Report is 5 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #246   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines         1364      1372    +8     
=========================================
+ Hits          1364      1372    +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[justinbhopper]

@wooorm I added some tests, please let me know if this looks good to you

[wooorm]

Hi again, thanks for your patience!

As mentioned, we don’t support ES5, so it feels weird to include a test for ES5 support.

You said that this problem also occurs more broadly. I wonder if that is the case. Can you come up with a test that shows that to be the case?

In unified itself, we don’t use CallableInstance on just any value: we only use it on our own Processor class, our copy function. Adding a console.log('descriptor:', [p, descriptor?.value]) to the code and running the tests, I get:

descriptor: [ 'length', 0 ]
descriptor: [ 'name', 'copy' ]
descriptor: [ 'length', 0 ]
descriptor: [ 'name', 'copy' ]
descriptor: [ 'length', 0 ]
descriptor: [ 'name', 'copy' ]
descriptor: [ 'length', 0 ]
descriptor: [ 'name', 'copy' ]
descriptor: [ 'length', 0 ]
descriptor: [ 'name', 'copy' ]
descriptor: [ 'length', 0 ]
descriptor: [ 'name', 'copy' ]
...

Perhaps still your change would make sense to callable-instance (and first patched here), but then I worry that by omitting non-configurable fields makes your current exception disappear, but will result in more exceptions, because now the fields are omitted.

Or if that isn’t the case, perhaps that we don’t need this descriptor patching loop here at all?

Finally: adding your tests but not including your patch to lib, the tests still run for me?

[justinbhopper]

@wooorm

As mentioned, we don’t support ES5, so it feels weird to include a test for ES5 support.

I added the tests per your request -- are you saying you want me to remove the tests now?

You said that this problem also occurs more broadly. I wonder if that is the case. Can you come up with a test that shows that to be the case?

Apologies, but I don't recall saying this. The problem only occurs when unified is used in a browser setting and is transpiled to ECMA 5.0. In fact, I can't even illustrate the bug in a test, because the issue only occurs on browsers and not in Node environment.

In unified itself, we don’t use CallableInstance on just any value: we only use it on our own Processor class, our copy function.

Yes, but the bug is specific to CallableInstance. The Processor class just happens to be the only class inheriting from it.

Perhaps still your change would make sense to callable-instance (and first patched here), but then I worry that by omitting non-configurable fields makes your current exception disappear, but will result in more exceptions, because now the fields are omitted.

I will make a change so we are instead checking the target object for an existing non-configurable property. This will ensure we're only skipping fields that are already defined and are non-configurable. This should ensure we're only skipping non-configurable properties that were defined on a brand new Function. It won't skip any non-configurable fields from the child class, so we won't be omitting anything from the class itself if they happen to define a non-configurable field.

Finally: adding your tests but not including your patch to lib, the tests still run for me?

That is because the tests run in Node, and Node does not define Functions with non-configurable properties like browsers do. I've only encountered this issue in a browser environment (Chrome). This sandbox illustrates the issue: https://playcode.io/1896334

[wooorm]

I added the tests per your request -- are you saying you want me to remove the tests now?

No, I appreciate a test. But I was wondering if there could be a different test.

Apologies, but I don't recall saying this. The problem only occurs when unified is used in a browser setting and is transpiled to ECMA 5.0.

See #245 (comment)

In fact, I can't even illustrate the bug in a test, because the issue only occurs on browsers and not in Node environment.

Right, and this makes me wary: how can we be sure it’s fixed? That this fix does not introduce different problems?

and Node does not define Functions with non-configurable properties like browsers do.

You can still define non-configurable properties in the tests though?

This sandbox illustrates the issue: playcode.io/1896334

What is the illustration? What behavior do you expect, what not, what is actual? I don’t get errors in Chrome or Safari with that sandbox.

It won't skip any non-configurable fields from the child class, so we won't be omitting anything from the class itself if they happen to define a non-configurable field.

I worry about this approach: this approach attempts to patch callable-instance generically. So, this patch should be possible to upstream. However, this patch just drops some fields without warning. As there is no test, I worry that that would introduce more problems.

I can think of two alternative solutions.

a) Ignore this dead/unneeded code. All the tests should still run, so it’s verified.

-       const names = Object.getOwnPropertyNames(value)
-
-       for (const p of names) {
-         const descriptor = Object.getOwnPropertyDescriptor(value, p)
-         if (descriptor) Object.defineProperty(apply, p, descriptor)
-       }
+       // Not needed for us in `unified`: we only call this on the `copy`
+       // function,
+       // and we don’t need to add its fields (`length`, `name`)
+       // over.
+       // See also: GH-246.
+       // const names = Object.getOwnPropertyNames(value)
+       //
+       // for (const p of names) {
+       //   const descriptor = Object.getOwnPropertyDescriptor(value, p)
+       //   if (descriptor) Object.defineProperty(apply, p, descriptor)
+       // }

b) Create an exception for these legacy and unneeded fields:

        for (const p of names) {
+         // Special non-configurable legacy function fields (see: GH-246).
+         if (p === 'arguments' || p === 'caller') continue;
+
          const descriptor = Object.getOwnPropertyDescriptor(value, p)

(we could also probably add length here, which is available on non-Chrome, and also seems useless; then the branch is covered on Node too).

[justinbhopper]

You can still define non-configurable properties in the tests though?

No, the bug is actually caused by attempts to redefine properties on the apply object, which is a fresh newly created Function:

There isn't any non-configurable fields that I can define that would cause this error. And now that we are checking the descriptor of the apply object's properties, the bug fix should be considered quite safe because it won't skip any non-configurable fields from the class/implementation.

What is the illustration? What behavior do you expect, what not, what is actual? I don’t get errors in Chrome or Safari with that sandbox.

Apologies, I accidentally saved the fix into the playground. If you look again using Chrome, you will see the original error.

I can think of two alternative solutions.

Your approach 'A' seems best to me. I didn't realize unified didn't actually need this cloning behavior. I agree, it seems unnecessary to attempt to clone properties of a function. Let me know if you would prefer I change the bug fix approach in this PR.

[wooorm]

Thanks so much for thinking with me on this!

Let me know if you would prefer I change the bug fix approach in this PR.

Yes please!

I prefer approach A as well.
With that approach, I believe all the existing tests are enough, so the new tests can be removed.

[justinbhopper]

@wooorm My pleasure! I've removed the tests and taken your suggested approach.

[wooorm]

released, thanks!