Skip to content

Conversation

@jamesricky
Copy link
Contributor

@jamesricky jamesricky commented Dec 15, 2025

Description

This will be used for helper functions for building HTML emails with React, such as the already included css helper.
Simple components built with React and/or MJML may also be added in future PRs.

Acceptance criteria

Open TODOs/questions

  • Add changeset

Further information

@jamesricky jamesricky self-assigned this Dec 15, 2025
@jamesricky jamesricky force-pushed the new-comet-mail-package branch from bade9e2 to 576a772 Compare December 15, 2025 10:15
@jamesricky jamesricky marked this pull request as ready for review December 15, 2025 10:23
@auto-assign auto-assign bot requested a review from johnnyomair December 15, 2025 10:23
This will be used for helper functions for building HTML emails with React, such as the already included `css` helper.
Simple components built with React and/or MJML may also be added in future PRs.
@jamesricky jamesricky force-pushed the new-comet-mail-package branch from 576a772 to 67cd778 Compare December 16, 2025 13:54
@johnnyomair
Copy link
Collaborator

What's the difference between this package and the mail-rendering package?

@johnnyomair
Copy link
Collaborator

Depending on what you intend to put into this package (is it only utils?) we should decide if we want to reserve the @comet/mail name for future usage (can't think of an example though).

Alternative names:

  • @comet/mail-utils
  • @comet/mail-rendering
  • @comet/mail-helper

@johnnyomair
Copy link
Collaborator

What's the difference between this package and the mail-rendering package?

Found an internal discussion: the mail-rendering package has peer dependencies on @comet/cms-site and styled-components, which we don't want.

@jamesricky
Copy link
Contributor Author

What's the difference between this package and the mail-rendering package?

It probably doesn't make sense to have both packages.
"Brevo Module" doesn't seem like the correct place for these things, as they are generic (maybe comet-specific) things that have nothing to do with Brevo.

We discussed this a while ago internally, I sent you the thread 🙂

Another reason was that we don't want/need the dependency on @comet/cms-site.
I'm not sure this point is still relevant though, since we have the separate @comet/site-react package now.

@jamesricky
Copy link
Contributor Author

Depending on what you intend to put into this package (is it only utils?) we should decide if we want to reserve the @comet/mail name for future usage (can't think of an example though).

Utils for now, likely react and/or react-mjml components in the future.
Basically things that help rendering the HTML for mails.

You're right, @comet/mail might be useful for other things in the future, that don't have anything to do with rendering the HTML (like sending-logic in the API or something 🤷🏼)

@comet/mail-rendering might be good 👍🏼

@thomasdax98 what do you think?

@jamesricky
Copy link
Contributor Author

What's the difference between this package and the mail-rendering package?

The mail-rendering package currently only exports a single NewsletterImageBlock component, most of the code seems to be the image logic which is now available in the @comet/site-react package anyway.

@johnnyomair
Copy link
Collaborator

It probably doesn't make sense to have both packages.

Agree'd. We should merge the @comet/brevo-mail-rendering package into this in the future.

@jamesricky
Copy link
Contributor Author

jamesricky commented Dec 19, 2025

It probably doesn't make sense to have both packages.

Agree'd. We should merge the @comet/brevo-mail-rendering package into this in the future.

That would probably make sense.

The image-logic seems to have been copied into @comet/brevo-mail-rendering. Therefore we could add @comet/site-react as a (peer-)dependency to the mail package, to avoid this and future code duplication.
Do you think that's a good idea? @johnnyomair

Maybe the name of the package should then be @comet/mail-react? 🤔
(Since it's basically like @comet/site-nextjs but for mails instead of nextjs 😄)

Maybe even an additional @comet/mail-mjml would make sense but I think that's overkill for now.

@johnnyomair
Copy link
Collaborator

Therefore we could add @comet/site-react as a (peer-)dependency to the mail package, to avoid this and future code duplication.
Do you think that's a good idea? @johnnyomair

I'm not sure about this, since site and mail could evolve into very different directions. Maybe a little copying is okay.

Maybe the name of the package should then be @comet/mail-react? 🤔
(Since it's basically like @comet/site-nextjs but for mails instead of nextjs 😄)

Yes, good idea!

Maybe even an additional @comet/mail-mjml would make sense but I think that's overkill for now.

Agree'd.

@jamesricky jamesricky marked this pull request as draft December 22, 2025 08:33
@jamesricky jamesricky changed the title Setup @comet/mail package Setup @comet/mail-react package Dec 22, 2025
@jamesricky jamesricky requested review from thomasdax98 and removed request for thomasdax98 December 22, 2025 08:49
@jamesricky jamesricky marked this pull request as ready for review December 22, 2025 08:50
@auto-assign auto-assign bot requested a review from johnnyomair December 22, 2025 08:51
@thomasdax98 thomasdax98 removed their request for review December 22, 2025 13:26
"main": "lib/index.js",
"scripts": {
"build": "pnpm run clean && tsc",
"clean": "rimraf lib",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The rimraf dependency appears to be missing.

"version": "8.10.0",
"description": "Utilities for building HTML emails with React",
"type": "commonjs",
"main": "lib/index.js",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The package.json should include a types and files field as well, see:

"types": "lib/index.d.ts",
"files": [
"bin/**/*.js",
"lib/*"
],
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added those, but without bin/**/*.js, as there is no bin directory created during build.
Is that correct? (I have no idea what this is used for 😅)

Comment on lines 25 to 28
"peerDependencies": {
"eslint": ">=9",
"prettier": ">=3"
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

This aren't peer dependencies. Which package did you choose as basis?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, should have noticed that, they are included in devDependencies anyway 😅

Which package did you choose as basis?

Mostly from site-react but partially from eslint-config because I thought that would be the smallest/simplest.

},
"devDependencies": {
"@comet/eslint-config": "workspace:*",
"@types/jest": "^29.5.14",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove this package.

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