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.

[lachrist]

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

[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]

Good work, thanks!

[lachrist]

FYI This file is used to build the agent and it does not contain any PropertyDefinition. But it is fine to add it as well here.

[dividedmind]

I figured, but it turned up in grep and I thought it won't hurt for completeness ;)

[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]

🎉 This PR is included in version 13.5.2 🎉

The release is available on:

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

Your semantic-release bot 📦🚀