fix: Support class features when transforming source code

[dividedmind]

appmap-agent-js uses @babel/parser to parse the ECMAScript code for transformation; the output generated by that library is not 100% estree-compliant [1]. It has an estree plugin that reverts the deviations; however, by default not all of them are supported (apparently for @babel/parser's own backward compatibility reasons). Fortunately it has a config setting that enables them.

One of these differences is that the standard node type of PropertyDefinition is called ClassProperty instead, which made the transformer unable to correctly handle ES classes.

[1] https://babeljs.io/docs/babel-parser#output

Fixes #199

[lachrist]

The official name is MethodDefinition and not PropertyDefinition. It would be good to verify in testing that astring is handling it.

> require("acorn").parse("class c { m () {} }").body[0].body.body[0].type
'MethodDefinition'

https://github.com/estree/estree/blob/master/es2015.md#methoddefinition

[lachrist]

It will be probably worthwhile to move to a babel-only format so that we do not have to rely to the less used astring and use @babel/generator instead. I tried doing that but it requires some non-trivial changes on the instrumentation visitors.

[dividedmind]

The official name is MethodDefinition and not PropertyDefinition. It would be good to verify in testing that astring is handling it.

> require("acorn").parse("class c { m () {} }").body[0].body.body[0].type
'MethodDefinition'

https://github.com/estree/estree/blob/master/es2015.md#methoddefinition

MethodDefinition is a different thing. This is about PropertyDefinition: https://github.com/estree/estree/blob/master/es2022.md#propertydefinition

[dividedmind]

Good work! Let's check that astring is handling PropertyDefinition first before merging.

I checked manually, it seems to work fine.

[dividedmind]

Good work! Let's check that astring is handling PropertyDefinition first before merging.

Added a test on the instrumentation side.

[dividedmind]

It will be probably worthwhile to move to a babel-only format so that we do not have to rely to the less used astring and use @babel/generator instead. I tried doing that but it requires some non-trivial changes on the instrumentation visitors.

I don't think that's a good idea unless it's really necessary. It will make it more difficult if we want to switch the parser and/or generator in the future.

[lachrist]

It will be probably worthwhile to move to a babel-only format so that we do not have to rely to the less used astring and use @babel/generator instead. I tried doing that but it requires some non-trivial changes on the instrumentation visitors.

I don't think that's a good idea unless it's really necessary. It will make it more difficult if we want to switch the parser and/or generator in the future.

Yeah it is really unfortunate that babel defined its own ast format. Let's keep it like this for the moment then.

[lachrist]

MethodDefinition is a different thing. This is about PropertyDefinition: https://github.com/estree/estree/blob/master/es2022.md#propertydefinition

Arf, I should keep up with the standard :s

[appland-release]

:tada: This PR is included in version 13.5.2 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: