-
Notifications
You must be signed in to change notification settings - Fork 87
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
chore: update react-email deps #7840
Conversation
de8910a
to
97ef4d6
Compare
c4f10a2
to
e821b13
Compare
Datadog ReportBranch report: ✅ 0 Failed, 1375 Passed, 1 Skipped, 3m 47.56s Total duration (2m 12.97s time saved) |
@@ -1079,19 +1093,30 @@ export class MailService { | |||
responseUrl, | |||
} | |||
|
|||
const html = render(MrfWorkflowEmail(htmlData)) | |||
console.log(render(MrfWorkflowEmail(htmlData))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray console.log
found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in 1541326
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few questions for my understanding, otherwise lgtm
"test:backend": "env-cmd -f __tests__/setup/.test-env jest", | ||
"test:backend:ci": "env-cmd -f __tests__/setup/.test-env jest --maxWorkers=2 --logHeapUsage --workerIdleMemoryLimit=0.2", | ||
"test:backend:watch": "env-cmd -f __tests__/setup/.test-env jest --watch", | ||
"test:backend": "env-cmd -f __tests__/setup/.test-env node --experimental-vm-modules node_modules/jest/bin/jest.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to understand this change - why did we need to add this experimental-vm-modules flag? based on my research, it has something to do with ESM vs commonJS modules - did the new bump in deps change to using ESM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
react-email@v1
introduced the use of dynamic imports. i.e., at runtime doing requires('somePackage')
. This is something that jest isn't very happy with.
For non-jest usage, we're running them through a different bundler / transpiler. While for tests, we're directly running it through jest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for more context, react email v3.0.2 does an ES modules dynamic import syntax const { default: reactDOMServer } = await import("react-dom/server");
. Since we are using jest, it does not support ES module syntax out of the box and requires the experimental-vm-modules
flag to support this ESM syntax
@@ -224,6 +226,7 @@ | |||
"maildev": "^2.1.0", | |||
"mockdate": "^3.0.5", | |||
"prettier": "^3.3.3", | |||
"react-dom": "^18.3.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to understand the usage of react-dom as well - I'm seeing usage of react-dom in some stories and index.tsx
However, they dont seem to be importing from react-email
previously, so im wondering why the removal of react-dom
from react-email/render
causes us to have to install react-dom
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rationale:
- react-email used to have react-dom as a dependency in v2.x.x but they switched it to a peer dependency in v3.0.2 (react-email uses react-dom internally)
Peer dependency means it is the responsibility of the 'peer' = aka FormSG in this case to install it as a dependency -> hence we add react-dom
to our dependencies.
Problem
Closes #7833, Closes #7834
Solution
Explicitly install
react-dom
asreact-email/render
has removedreact-dom
from its deps, i.e., it is no longer transitively installed, thus we have to add it ourselves.Wrapped
render
withfromPromise
as it is now async.Breaking Changes