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 async html transformer function #103

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

peterlama
Copy link
Contributor

Having an async function to transform the html allows for more complex transformations, such as inserting data fetched from a database.

I made this change for my own use case, but thought I would open a PR in case it would be helpful for others.

Copy link
Owner

@szymmis szymmis left a comment

Choose a reason for hiding this comment

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

Hi @peterlama, thanks a lot for sharing your work!

I can totally see the use case, so great idea to implement that. The only issue right now is that tests don't pass and I see why. You need to make sure to await getTransformedHTML also in the injectIndexMiddleware function here.

Besides that, everything look great! Waiting for the change and we can ship it! 🎉

@peterlama peterlama force-pushed the feature/async-transformer branch from 9c5a0c4 to 89caf9f Compare December 30, 2023 20:53
@szymmis
Copy link
Owner

szymmis commented Dec 31, 2023

That looks great! The one last thing is to add note about new async feature of transform function to README, are you willing to do that also?

Having an async function to transform the html allows for more complex transformations, such as inserting data fetched from a database
@peterlama peterlama force-pushed the feature/async-transformer branch from 89caf9f to 70b1338 Compare December 31, 2023 20:26
@peterlama
Copy link
Contributor Author

How's that?

@szymmis
Copy link
Owner

szymmis commented Jan 2, 2024

That's perfect 🎉, thank you again for your contribution!

@szymmis szymmis merged commit 26f0721 into szymmis:master Jan 2, 2024
2 checks passed
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.

2 participants