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

feat: Abstracted content engine #2003

Merged
merged 2 commits into from
Nov 16, 2022
Merged

Conversation

Swahjak
Copy link
Contributor

@Swahjak Swahjak commented Nov 10, 2022

What change does this PR introduce?

Initial thoughts on how the content engine might be abstracted to allow it to be extended / replaced.

Why was this change needed?

Closes #2002

Other information (Screenshots)

@Swahjak Swahjak changed the title feat: intial throughts on abstracted content engine feat: Abstracted content engine Nov 10, 2022
Copy link
Contributor

@p-fernandez p-fernandez left a comment

Choose a reason for hiding this comment

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

Good abstraction. Besides a pair of comments you would also need to update the test file: https://github.com/novuhq/novu/blob/2d3050a13b5dfc77b0d66e34ed2eac8a25299631/packages/stateless/src/lib/content/content.engine.spec.ts that is dependant in the changes you are proposing.

@Swahjak
Copy link
Contributor Author

Swahjak commented Nov 13, 2022

Good abstraction. Besides a pair of comments you would also need to update the test file: https://github.com/novuhq/novu/blob/2d3050a13b5dfc77b0d66e34ed2eac8a25299631/packages/stateless/src/lib/content/content.engine.spec.ts that is dependant in the changes you are proposing.

Didn't get to that just yet, but I will update the tests as well. Thanks for the review, I'll try and update the PR in the upcoming week 👍

@Swahjak
Copy link
Contributor Author

Swahjak commented Nov 15, 2022

@p-fernandez to what extend, and especially in which files, should I test the ability to change the engine? Should I add tests to both the email and sms handler (spec)? I already updated the content.engine.spec.ts.

@p-fernandez
Copy link
Contributor

@p-fernandez to what extend, and especially in which files, should I test the ability to change the engine? Should I add tests to both the email and sms handler (spec)? I already updated the content.engine.spec.ts.

Not sure if I understood you correctly but I do not think you should test what happens if changing the engine (moving from Handlebars to a different library).
If you meant if you have to test changes in email.handler.ts and sms.handler.ts, I consider it shouldn't be needed to do anything extra, as the current tests implement the templating functionality and your changes will be validated if they still pass as they are.

Copy link
Contributor

@p-fernandez p-fernandez left a comment

Choose a reason for hiding this comment

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

🌟

@scopsy scopsy merged commit 111e752 into novuhq:next Nov 16, 2022
@Swahjak Swahjak deleted the feature-content-engine branch November 16, 2022 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 Feature: Allow content engine to be extended or replaced
3 participants