feat: trigger pre connection deletion script on connection deletion

NangoHQ / nango

A single API for all your integrations.

https://www.nango.dev

Other

4.83k stars 442 forks source link

feat: trigger pre connection deletion script on connection deletion #3027

Closed TBonnin closed 1 week ago

TBonnin commented 1 week ago

Describe your changes

Trigger pre-deletion-script when connection is deleted

Issue ticket number and link

https://linear.app/nango/issue/NAN-1962/connect-lifecycle-scripts

Checklist before requesting a review (skip if just adding/editing APIs & templates)

[ ] I added tests, otherwise the reason is:
[ ] I added observability, otherwise the reason is:
[ ] I added analytics, otherwise the reason is:

linear[bot] commented 1 week ago

NAN-1962 Connect lifecycle scripts

bodinsamuel commented 1 week ago

You haven't merged, a bit harder to review

TBonnin commented 1 week ago

You haven't merged, a bit harder to review

Sorry there were some conflicts and github couldn't automerge. It should be fine now @bodinsamuel

nalanj commented 1 week ago

Tossing here since I'm not quite to really reviewing yet. When I ran nango generate in the directory to get the event script I accidentally called the script: unauthenticated/pre-connection-deletion because I was just troubleshooting, and it really didn't like that.

Error: ENOENT: no such file or directory, open '/Users/nalanj/Source/NangoHQ/nango-integrations/unauthenticated/on-events/unauthenticated/pre-connection-deletion.ts'

May be worth us blocking slashes in those?

nalanj commented 1 week ago

In testing this, I'm also seeing that it would be nice if we could somehow surface these scripts existing in the UI so it's easy to know that everything is working as expected.

nalanj commented 1 week ago

Also verified this ran! The log entry looks like this:

Should that have the name of the on-event script there?

TBonnin commented 1 week ago

Tossing here since I'm not quite to really reviewing yet. When I ran nango generate in the directory to get the event script I accidentally called the script: unauthenticated/pre-connection-deletion because I was just troubleshooting, and it really didn't like that.
Error: ENOENT: no such file or directory, open '/Users/nalanj/Source/NangoHQ/nango-integrations/unauthenticated/on-events/unauthenticated/pre-connection-deletion.ts'
May be worth us blocking slashes in those?

good catch. fixed in https://github.com/NangoHQ/nango/pull/3027/commits/e16fb503d388963bbf0416b0a529a44212cd9265

TBonnin commented 1 week ago

Also verified this ran! The log entry looks like this:

Should that have the name of the on-event script there?

maybe. I have followed the same pattern used in post-connection-scripts where if multiple scripts they are run as part of the same log operation. In this case which scripts should be displayed? I have a follow-up ticket to first decide if we want to split multiple scripts as multiple logs operations and in this case we could show the script name. Wanted to discuss the UX implications with @bastienbeurier @bodinsamuel

TBonnin commented 1 week ago

Works well 👍🏻 Nitpick, the onEvent prop does not reflect the event name but the script name which is confusing (I named mine pre.ts )

good catch. renamed the field to onEventScript in https://github.com/NangoHQ/nango/pull/3027/commits/566b883c42a8e32a7d2e4325d21f1f6ff7ab24a6

TBonnin commented 1 week ago

In testing this, I'm also seeing that it would be nice if we could somehow surface these scripts existing in the UI so it's easy to know that everything is working as expected.

I know. I will do that in a following PR because it requires a bit of refactoring