-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add additional ts signatures and react-based types to detect #6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add additional ts signatures and react-based types to detect #6
Conversation
|
Also, noticed CI is not calling new test code. I think it is because of cache. |
|
@skarim lmk what you think! |
|
@ahnopologetic thanks for contributing! need to do a deeper read, but looks great at first glance! curious, did the existing version not work with react for you? i did some spot tests on react codebases (although not in the test suite) and did see events get extracted. is there a particular format you are looking to parse? |
|
@skarim Hi. Yes, I saw some of them works, but it does not extract some simple usecases like this: mixpanelAnalytics.trackUserScenario('sign_up: completed', {And the mixpanelAnalytics is declared like this export class MixpanelAnalytics {
app: MixpanelModule;
debug: boolean;
constructor(app: MixpanelModule, debug: boolean = false) {
this.app = app;
this.debug = debug;
}
trackUserScenario(eventName: string, properties: Record<string, any> = {}) {
if (properties?.is_user_scenario === false) {
console.warn(
'MixpanelAnalytics.trackUserScenario(): is_user_scenario is false, use track instead',
);
}
this.app.track(eventName, {
...properties,
is_user_scenario: true,
});
}
...I tried to pull them up using customFunction option, but it turns out customFunction only checks if it is identifier (typescript AST), not other usecases like class property/method, and so forth. Thus, I added this part in isCustomFunction function: const canBeCustomFunction = ts.isIdentifier(node.expression) ||
ts.isPropertyAccessExpression(node.expression) ||
ts.isCallExpression(node.expression) || // For chained calls like getTracker().track()
ts.isElementAccessExpression(node.expression) || // For array/object access like trackers['analytics'].track()
(ts.isPropertyAccessExpression(node.expression?.expression) && ts.isThisExpression(node.expression.expression.expression)); // For class methods like this.analytics.track()I think this issue is specific to React usecases because they are usually wrapped by custom arrow functions, useCallbacks, and so on. Hope it helps you have a better understanding for this PR. |
skarim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahnopologetic left some comments for a few areas that could be improved. open to your thoughts either way. lemme know when you have a chance to review/update and i'll get this into the next release!
|
@skarim Just to make sure, resolved your all RCs and passed the tests locally. 👍 |
skarim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm ✅
thanks @ahnopologetic!
Adding a feature that parse react-based codebase as well.
mixpanelAnalytics.trackwhich is a property of a custom class).