-
Notifications
You must be signed in to change notification settings - Fork 1
Implement translations from old event types to new ones #99
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
base: main
Are you sure you want to change the base?
Conversation
db603ae to
f462b19
Compare
radovanjorgic
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.
Update PR title to something understandable to 3P developers (old to new event types instead of E2DR to DR2E). Please verify this works on template, asana and maybe even release one beta version so other connectors can test if their connectors still work. I would be super careful with merging this.
… inside of spawn.
37917c2 to
de2c3cd
Compare
radovanjorgic
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.
Much better, few nits.
ef82227 to
0fc8fe0
Compare
0fc8fe0 to
d307312
Compare
src/workers/spawn.ts
Outdated
| function getCallerDirectory(): string { | ||
| const originalPrepareStackTrace = Error.prepareStackTrace; | ||
|
|
||
| Error.prepareStackTrace = (_, stack) => stack; | ||
| const err = new Error(); | ||
| Error.captureStackTrace(err); | ||
| const stack = err.stack as unknown as NodeJS.CallSite[]; | ||
| Error.prepareStackTrace = originalPrepareStackTrace; | ||
|
|
||
| // Iterate through the stack to find the first file that's not part of the SDK | ||
| for (const callSite of stack) { | ||
| const filename = callSite.getFileName(); | ||
| if ( | ||
| filename && | ||
| !filename.includes('@devrev/ts-adaas') && | ||
| !filename.includes('adaas-sdk') | ||
| ) { | ||
| return path.dirname(filename); | ||
| } | ||
| } | ||
| return path; | ||
|
|
||
| // Fallback to __dirname if we can't determine the caller | ||
| return __dirname; |
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.
I have had to reimplement the automatic detection of the directory the spawn is being called from.
This works by reading the callstack and navigating up just far enough to get out of the SDK.
This might not be perfect and it could fail on some edge-cases (that I was unable to think of), but it has been tested both locally and in Lambda.
If we override the getWorker, that will be appended to the path where we called spawn, if it's not provided, the pre-defined paths will be appended to the path.
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.
TBH I don't like this. This way of stack inspection seems pretty fragile and unreliable to me. I have a feeling it might break with bundlers, if the package name differs and so on. Also, this is pretty non-standard approach for JavaScript.
On the other side, I think there won't be many cases where users want to override those paths, so can we rather require user to pass baseDirectory in the options alongside with workerPathOverridings. What is __dirname in this case?
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.
I have replaced it with a field in spawn, that allows user to pass in the current script directory.
src/common/event-type-translation.ts
Outdated
| [EventType.UnknownEventType]: EventType.UnknownEventType, | ||
| }; | ||
|
|
||
| const normalized = eventTypeMap[eventTypeString]; |
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.
Use translated instead normalized all over the code
| isLocalDevelopment?: boolean; | ||
| timeout?: number; | ||
| batchSize?: number; | ||
| workerPathOverrides?: Partial<Record<EventType, string>>; |
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.
We still use Partial here and don't have separate interface for this?
src/types/workers.ts
Outdated
| export interface GetWorkerPathInterface { | ||
| event: AirdropEvent; | ||
| connectorWorkerPath?: string | null; | ||
| callerDir?: string | null; |
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.
Document this a bit through JSDoc please, what is callerDir?
src/workers/spawn.ts
Outdated
| function getCallerDirectory(): string { | ||
| const originalPrepareStackTrace = Error.prepareStackTrace; | ||
|
|
||
| Error.prepareStackTrace = (_, stack) => stack; | ||
| const err = new Error(); | ||
| Error.captureStackTrace(err); | ||
| const stack = err.stack as unknown as NodeJS.CallSite[]; | ||
| Error.prepareStackTrace = originalPrepareStackTrace; | ||
|
|
||
| // Iterate through the stack to find the first file that's not part of the SDK | ||
| for (const callSite of stack) { | ||
| const filename = callSite.getFileName(); | ||
| if ( | ||
| filename && | ||
| !filename.includes('@devrev/ts-adaas') && | ||
| !filename.includes('adaas-sdk') | ||
| ) { | ||
| return path.dirname(filename); | ||
| } | ||
| } | ||
| return path; | ||
|
|
||
| // Fallback to __dirname if we can't determine the caller | ||
| return __dirname; |
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.
TBH I don't like this. This way of stack inspection seems pretty fragile and unreliable to me. I have a feeling it might break with bundlers, if the package name differs and so on. Also, this is pretty non-standard approach for JavaScript.
On the other side, I think there won't be many cases where users want to override those paths, so can we rather require user to pass baseDirectory in the options alongside with workerPathOverridings. What is __dirname in this case?
Description
Migrated event type names from old E2DR to DR2E naming convention.
Added a simple function for easier translation and migration.
Connected Issues
Checklist
npm run testOR no tests needed.npm run test:backwards-compatibility.npm run lint.