Optional chaining rewriting by Hammerhead causes unhandled exception

[lg-kialo]

What is your Scenario?

We are testing our SPA with TestCafe and after changing our JS bundling code to target newer browsers, most of our tests are failing due to unhandled exceptions that crash the app.
After investigation we identified the root of the problem in how Hammerhead rewrites our Javascript.

More specifically given this input code:

renderClaimFromSameDiscussionForDragImage() {
  const e = this.props.clipboardData?.location.id || this.props.effectiveLocationId;
}

Hammerhead transpiles it to

renderClaimFromSameDiscussionForDragImage() {
  const e = __get$(this.props.clipboardData, 'location', true).id || this.props.effectiveLocationId;
}

During runtime, this.props.clipboardData is undefined, which would not be an issue for the original version, but the rewrite throws the TypeError: __get$(...) is undefined exception.

Looking at #2714, it seems like support for optional chaining should be part of TestCafe already. So not sure what's happening here.

What is the Current behavior?

Our tests fail due to Hammerhead rewriting the Javascript bundles

What is the Expected behavior?

Hammerhead doesn't mess with our Javascript :)

What is your public website URL? (or attach your complete example)

See above

What is your TestCafe test code?

It doesn't matter

Your complete configuration file

No response

Your complete test report

No response

Screenshots

No response

Steps to Reproduce

See above

TestCafe version

2.2.0

Node.js version

No response

Command-line arguments

Browser name(s) and version(s)

Firefox 110

Platform(s) and version(s)

Ubuntu 22.04

Other

testcafe-hammerhead version 28.2.5

[lg-kialo]

I saw just now DevExpress/testcafe#7387 and DevExpress/testcafe#7424, I should try upgrading testcafe-hammerhead to the latest version first which should have the changes from #2849

EDIT: Upgraded testcafe-hammerhead to 29.0.0 (forcing the resolutions to only use that version) and the issue is still there. The code gets rewritten in the same way.

[Aleksey28]

Hi @lg-kialo,

I managed to reproduce the issue. We will update this thread once we have news.

As a workaround, you can use the --experimental-proxyless option. We are actively working on this mode.

[lg-kialo]

Hi @lg-kialo,

I managed to reproduce the issue. We will update this thread once we have news.

As a workaround, you can use the --experimental-proxyless option. We are actively working on this mode.

Thank you for looking into this. Unfortunately the proxyless mode does not suit us, we don't use Chrome because I think we have too many blinkers there

[Aleksey28]

I see. The only thing I can suggest is to add an optional chaining operator to all properties after the first one. For example:

this.props.clipboardData?.location?.id

It will help you for a while.

[bbutel]

I see. The only thing I can suggest is to add an optional chaining operator to all properties after the first one. For example:

this.props.clipboardData?.location?.id

It will help you for a while.

Hi,

I have the same issue on my side.
For me, it's

return e?.myObject[X] ?? 0

# transformed into 

__get$(e?.myObject, X) ?? 0

Do you have a patch we could apply on hammerhead instead of your proposal to change our application on optional chaining management.

By the way, I cannot use proxyless mode because it seems that some breaking changes come with it (run does not work at all in proxyless mode on my side. I have no time to investigate)

Thx a lot

[Aleksey28]

Hi @Butel,

The issue you encountered is different from the original post. In the original post, the main problem is the `location' keyword. We have a special handling mechanism for it. You don't have this keyword

Your example works as expected. Please describe the problem you encountered with this conversion.

[bbutel]

Thx for your answer.

I think it is the same issue.

Indeed, in original description, it is said that "this.props.clipboardData?.location.id" is transformed into __get$(this.props.clipboardData, 'location', true).id

In both cases, the transformation does not take care about the possibility that first parameter could be undefined and led to an exception instead of returning undefined as expected.

To me, it is an issue about the optional chaining management with hammerhead proxy.

So it would be great to have a fix to support all kind of optional chaining and not opening an issue each time we are facing to an unpexpected behavior.

thx a lot

[Aleksey28]

JavaScript is permanently improving, that's why we cannot cover all cases once and forever. We improve testcafe-hammerhead behavior step by step with available resources.

Regarding your case, I don't understand which exactly unexpected behavior you faced. All that I tried with your example worked as expected in the browser. We can't just throw away this transpilation, because it is necessary for correct work testcafe-hammerhead, but we make it work as expected.

[bbutel]

Thx a lot for your answer.
My issue is the same as mentionned by @lg-kialo

# e is undefined
e?.myObject[X] ?? 0 # return undeifned with optional chaining
# but hammerhead change that into 
__get$(e?.myObject, X) ?? 0 # here, an exception is raised. 

Here the patch to ugly solve my issue:

diff --git a/node_modules/testcafe-hammerhead/lib/client/hammerhead.js b/node_modules/testcafe-hammerhead/lib/client/hammerhead.js
index 2b7fea5..5e52241 100644
--- a/node_modules/testcafe-hammerhead/lib/client/hammerhead.js
+++ b/node_modules/testcafe-hammerhead/lib/client/hammerhead.js
@@ -4130,6 +4130,7 @@
 	    directive,
 	    extra;
 
+
 	var Syntax = {
 	    AssignmentExpression:     'AssignmentExpression',
 	    AssignmentPattern:        'AssignmentPattern',
@@ -21486,7 +21487,9 @@
 	    PropertyAccessorsInstrumentation._propertyGetter = function (owner, propName, optional) {
 	        if (optional === void 0) { optional = false; }
 	        if (isNullOrUndefined(owner) && !optional)
-	            PropertyAccessorsInstrumentation._error("Cannot read property '".concat(propName, "' of ").concat(inaccessibleTypeToStr(owner)));
+						// ugly fix for https://github.com/DevExpress/testcafe-hammerhead/issues/2858
+						return undefined
+	          //PropertyAccessorsInstrumentation._error("Cannot read property '".concat(propName, "' of ").concat(inaccessibleTypeToStr(owner)));
 	        if (typeof propName === 'string' && shouldInstrumentProperty(propName)) {
 	            if (optional && isNullOrUndefined(owner))
 	                return void 0;

thx a lot

[Aleksey28]

Thank you for your description. I managed to reproduce this problem. It's not the same as the initial one, so please create a separate issue for it.

[bbutel]

thx a lot. Here the new issue: #2891

[github-actions]

This issue has been automatically marked as stale because it has not had any activity for a long period. It will be closed and archived if no further activity occurs. However, we may return to this issue in the future. If it still affects you or you have any additional information regarding it, please leave a comment and we will keep it open.

[gg-kialo]

The issue seems to be that the transformation treats MemberExpression in isolation in property-get.ts. But MemberExpressions that are part of a ChainExpression (estree docs) have semantics that should to be performed on the level of the ChainExpression.

In order to support the correct semantics of optional chaining, I think the transformed code will need to look like the output of how a transpiler would support optional chaining in old browsers.

[Bayheck]

Hello,

The reported issue has a TestCafe workaround available when Native Automation is enabled. Due to this, the priority of this issue is relatively low. That means that we don’t have immediate plans to work on this task, and we'll be closing this issue.

However, if you'd like to contribute and have a solution in mind, feel free to create a pull request (PR) addressing this issue. We would appreciate any contributions and will gladly review any PRs submitted.

Thank you for your understanding and for your interest in improving our platform.