VulcanJS / Vulcan

🌋 A toolkit to quickly build apps with React, GraphQL & Meteor
http://vulcanjs.org
MIT License
7.98k stars 1.89k forks source link

Improvement to the VulcanEmail API #2048

Closed PolGuixe closed 6 years ago

PolGuixe commented 6 years ago

I think that sending the result of the query straight to the email template is just too direct. I would like to have a method to modify the results of the query e.g. sorting data, etc.

I see different options:

  1. Make the results go to the data method.
  2. Add a new method e.g. resultsModifier

@SachaG what do you think?

SachaG commented 6 years ago

I think I would rather have the query return the right results in the first place, since GraphQL gives you a lot of flexibility?

PolGuixe commented 6 years ago

I think sometimes is good if we can format certain data on the other results side of the query. This way we can reuse queries and resolvers with simple modifications on the results.

Example 1: I store data as a Date type and I want to print the Date in a different format in the email template.

Example 2: I want to filter and split de results to display them in a different list within the same email.

I sure there are workarounds e.g. example 1: using {{moment}} helper - example 2: using specific resolver for each list.

Adding a way to modify the results from the query just makes some cases a lot easier to implement and make some more reusable and easier to maintain.

SachaG commented 6 years ago

OK, we could add a callback at some point in the code then. Although for 1. specifically I think GraphQL arguments are a great way to handle that (postedAt(format: "short") for example).

Apollinaire commented 6 years ago

I agree that letting us put our own js between the query and the template would be appreciated, for instance If i'm using the default resolvers I don't want to write another resolver to deal with formatting arguments. I think this kind of thing should be done after the resolution of a query, to keep a unified API.

Since we're talking here about the emails subject, let me piggyback this issue to add some suggestions/feedback from my experience:

SachaG commented 6 years ago

Yeah I think generally Vulcan's email features should be considered a version 0.x. A lot of it is legacy code from Telescope so I'm sure there's lots that could be improved. We should also keep an eye out for existing Node solutions because email is a fairly self-contained thing. We should also stop relying on Meteor's own email sending eventually and just use a "native" Node package if there's an equivalent?

PolGuixe commented 6 years ago

I've managed to solve all my email issues using handlebars helpers and graphql.

I still think that being able to manipulate the data from the query is still useful. So in the short-term, we can find a way to do that. As I mentioned above the most straightforward way for me would be to add a method to registerEmail. I don't see the need to add callbacks, as each email is quite unique.

In the mid/long-term I would look to migrate to React based approach rather than using Handlebars, and also migrating to a non-Meteor solution. Doing a quick online search I've got these options:

I don't know which one would be easier to implement and I guess that we would need to render React on the server to send it as the email.

SachaG commented 6 years ago

Actually it might not be documented but there's already a data() function that you can pass:

https://github.com/VulcanJS/Vulcan/blob/devel/packages/vulcan-email/lib/server/email.js#L120-L121

Here's how I use it to add the current date to my email variables for example:

    data() {
      return {date: new Date()};
    },

It's not quite what you're asking for because it doesn't currently know about the query results, but maybe we could tweak it? Or yeah add an additional function here.

PolGuixe commented 6 years ago

My first thought was to tweak this function so it receives the query results.

I think it would be a good way to implement this feature.