Skip to content
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

Support hosted triggers #111

Merged
merged 3 commits into from
Jun 25, 2024
Merged

Support hosted triggers #111

merged 3 commits into from
Jun 25, 2024

Conversation

peterszerzo
Copy link
Collaborator

@peterszerzo peterszerzo commented Jun 24, 2024

@jakub-nlx also resolves your previous feedback about the DOMContentLoaded event being moved inside the SDK method, this will allow the setup script to be a bit simpler.

@peterszerzo peterszerzo requested a review from jakub-nlx as a code owner June 24, 2024 07:44
Copy link
Collaborator

@jakub-nlx jakub-nlx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closes #93.

*/
triggers: Triggers;
triggers?: Triggers;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to pass in an explicit url rather than just null? That also gives them the opportunity to self-host if they so please?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should check with the broader team whether the custom hosting functionality should be an option (has implications on whether we want to support users with that).

The hosting we will do is automatically configured under those specified URL's at all times, what is optional here is the triggers themselves (mostly for backwards-compatibility), which they can get in a two-liner if they want.

packages/journey-manager/src/index.ts Outdated Show resolved Hide resolved
jakub-nlx
jakub-nlx previously approved these changes Jun 24, 2024
Comment on lines 222 to 223
? "https://triggers.dev.nlx.ai"
: "https://triggers.nlx.ai";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's https://triggers.dev.mm.nlx.ai and https://triggers.mm.nlx.ai

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vlad31 oh good point, updated!

@peterszerzo peterszerzo merged commit 032e5fb into main Jun 25, 2024
4 checks passed
@peterszerzo peterszerzo deleted the hosted-triggers branch June 25, 2024 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants