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

[Bug?]: Redwood Studio can't load email templates (SWC) #9620

Closed
1 task done
raph90 opened this issue Dec 3, 2023 · 18 comments
Closed
1 task done

[Bug?]: Redwood Studio can't load email templates (SWC) #9620

raph90 opened this issue Dec 3, 2023 · 18 comments
Assignees
Labels
bug/confirmed We have confirmed this is a bug topic/mailer RedwoodJS Mail and Mailer related

Comments

@raph90
Copy link
Contributor

raph90 commented Dec 3, 2023

What's not working?

Due to some issue of filesystem access in the swc library, Redwood Studio can't dynamically load my React Email templates.

The error stack is this:

Error updating mailer and mail templates:
Error: Fallback bindings does not support filesystem access
    at Compiler.parseFileSync (my_dir/node_modules/@redwoodjs/studio/node_modules/@swc/core/index.js:163:19)
    at Object.parseFileSync (my_dir/node_modules/@redwoodjs/studio/node_modules/@swc/core/index.js:332:21)
    at getMailTemplateComponents (my_dir/node_modules/@redwoodjs/studio/dist/api/mail/index.js:206:19)
    at updateMailTemplates 

How do we reproduce the bug?

  • new redwood build on 6.4.2
  • setup mailer
  • add React Email configuration
  • start Redwood Studio

What's your environment? (If it applies)

System:
    OS: macOS 14.0
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.2.0 - /private/var/folders/vf/nj_jbnqs6s97151rx0vf36zh0000gn/T/xfs-36609f3c/node
    Yarn: 3.5.0 - /private/var/folders/vf/nj_jbnqs6s97151rx0vf36zh0000gn/T/xfs-36609f3c/yarn
  Databases:
    SQLite: 3.39.5 - /usr/bin/sqlite3
  Browsers:
    Chrome: 119.0.6045.199
    Safari: 17.0
  npmPackages:
    @redwoodjs/auth-auth0-setup: 6.4.2 => 6.4.2
    @redwoodjs/cli-storybook: 6.4.2 => 6.4.2
    @redwoodjs/core: 6.4.2 => 6.4.2
    @redwoodjs/studio: 6.4.2 => 6.4.2

Are you interested in working on this?

  • I'm interested in working on this
@raph90 raph90 added the bug/needs-info More information is needed for reproduction label Dec 3, 2023
@Tobbe
Copy link
Member

Tobbe commented Dec 4, 2023

Assigning this one to you @Josh-Walker-GM, because mailer! 🙂

@Tobbe Tobbe added the topic/mailer RedwoodJS Mail and Mailer related label Dec 4, 2023
@raph90
Copy link
Contributor Author

raph90 commented Dec 5, 2023

Hi @Josh-Walker-GM, this is what I've worked out so far.

The SWC issue relates to dev dependencies, it can be fixed by installing the following as dev dependencies:

"@swc/core-darwin-arm64": "1.3.60",
    "@swc/core-darwin-x64": "1.3.60",
    "@swc/core-linux-arm64-gnu": "1.3.60",
    "@swc/core-linux-arm64-musl": "1.3.60",
    "@swc/core-linux-x64-gnu": "1.3.60",
    "@swc/core-linux-x64-musl": "1.3.60",
    "@swc/core-win32-arm64-msvc": "1.3.60",
    "@swc/core-win32-ia32-msvc": "1.3.60",
    "@swc/core-win32-x64-msvc": "1.3.60",

However, there's then an issue with this line (216 in the compiled index.js from @redwoodjs/studio/dist/api/mail/index.js)
const hasParams = exportedComponents[i].declaration.params.length > 0;
params is actually undefined on the object that comes back from the AST generated by SWC - I'm pretty sure this isn't a versioning issue (I made sure all my versions were the same as the ones defined in redwood), so I'm not quite sure how to proceed with it.

If I make params optional:
const hasParams = exportedComponents[i].declaration.params?.length > 0; then the errors go away, but the preview doesn't work for emails when I export them like this:

import {
  Html,
  Text,
  Hr,
  Body,
  Head,
  Tailwind,
  Preview,
  Container,
  Heading,
} from '@react-email/components'

type ExampleEmailProps = {
  name: string
}

export const WelcomeEmail = (props: ExampleEmailProps) => {
  return (
    <Html lang="en">
      <Head />
      <Preview>An example email</Preview>
      <Tailwind>
        <Body className="mx-auto my-auto bg-white font-sans">
          <Container className="mx-auto my-[40px] rounded border border-solid border-gray-200 p-[20px]">
            <Heading className="mx-0 my-[30px] p-0 text-center text-[24px] font-normal text-black">
              Example Email
            </Heading>
            <Text className="text-[14px] leading-[24px] text-black">
              This is an example email which you can customise to your needs.
            </Text>
            <Hr className="mx-0 my-[26px] w-full border border-solid border-[#eaeaea]" />
            <Text className="text-[12px] leading-[24px] text-[#666666]">
              {props.name}
            </Text>
          </Container>
        </Body>
      </Tailwind>
    </Html>
  )
}

and use them like this:

export const sendWelcomeEmail = async (
  recipient: string,
) => {
  await mailer.send(WelcomeEmail({ name: 'hi' }), {
    to: recipient,
    subject: 'New Email',
    from: 'me@gmail.com'
  })
}

In the preview in Redwood studio I get 'component is not a Function'...
Screenshot 2023-12-05 at 08 17 02

Lmk if you'd like to try to work through this one together. Thanks!

@Josh-Walker-GM
Copy link
Collaborator

Josh-Walker-GM commented Dec 6, 2023

Thanks for providing such detail @raph90!

Sorry I haven't gotten to this sooner. My initial reaction to your points are:

  1. Can all those platform specific dependencies go in as optional dependencies or are they already treated like that? I'll need to reread the SWC docs on this.
  2. I'll have to pull out astexplorer.net to examine what I messed up with the parameter extraction.

If you'd like to make a PR for this then I'd be more than happy to help out with it and review it. I do hope to spend some time on this tomorrow and if I get to the bottom of it I'll likely make a PR. I can leave it and give you a few days to tackle this one too if you'd like but if you could let me know in the next day then that would be great.

Thanks again for the details it's really great when people take the time to document things as clearly as you have, awesome!

@raph90
Copy link
Contributor Author

raph90 commented Dec 6, 2023

Hi @Josh-Walker-GM, I'm afraid I don't think I'd be able to get round to a PR for this at the moment - but very happy to talk through stuff here, since I would really love to start contributing to RW.

  1. Can all those platform specific dependencies go in as optional dependencies or are they already treated like that? - Yes I think they can, the SO post where I found that solution used npm -optional (I think) and I wasn't sure of the yarn equivalent

  2. Yeah I'm not sure what the issue is here - I'm pretty sure I did everything correctly / downloaded the correct packages.

As an additional question - if you wanted to debug / breakpoint through Studio, what would be the correct way to do that? Would you have a launch.json for VS Code that you might be able to share? Or is just logging the way do you think?

I tried loading up redwood and doing the RWFW command on my project, but I wasn't able to get any breakpoints to catch in the api/mail code.

No worries about the write up, many thanks for looking into this!

@Josh-Walker-GM Josh-Walker-GM added bug/confirmed We have confirmed this is a bug and removed bug/needs-info More information is needed for reproduction labels Dec 6, 2023
@raph90
Copy link
Contributor Author

raph90 commented Dec 6, 2023

@Josh-Walker-GM I'm going to try and have a go at this this evening, I'll let you know how I get on

@Josh-Walker-GM
Copy link
Collaborator

Okay perfect!

I couldn't reproduce the issue with the dependencies but I do think we should add them to the optional dependencies as discussed above.

I also noticed that I left comments in the original code about what "style" of email component code we support in the ast parsing. Looks like those point to why you saw the "component is not a function" error.

@raph90
Copy link
Contributor Author

raph90 commented Dec 7, 2023

@Josh-Walker-GM here's a PR - let me know what you think, trying things my end and it does seem to work...

#9639

EDIT
I've also just seen your comments 🤦

  // TODO: Support `const X = () => {}; export default X;`
  // TODO: Support `export default function X () => {}`
  // TODO: Support `export default () => {}`

Maybe you'd like me to add those to this PR? It would be a bit more work - an alternative is I also just add a line to the docs saying which syntaxes are supported (after spending a little bit of time working with ASTs, that would be my preference 😆

EDIT AGAIN - It's not the end of the world, I'll add support for those other syntaxes.

@raph90
Copy link
Contributor Author

raph90 commented Dec 7, 2023

Hi @Josh-Walker-GM , I'm still working on this but I'm a bit worried about whatever implementation I propose, because we might be seeing different AST syntax. For example, for your export function code (which works), you have:

try {
        switch (param.pat.type) {
          case 'ObjectPattern':
            propsTemplate = `{${param.pat.properties
              .map((p: any) => {
                return `\n  "${p.key.value}": ?`
              })
              .join(',')}\n}`
            break
        }
      } catch (_error) {
        // ignore for now
      }

If I console log param there, it looks like this:

{
  type: 'Parameter',
  span: { start: 198, end: 222, ctxt: 0 },
  decorators: [],
  pat: {
    type: 'Identifier',
    span: { start: 198, end: 222, ctxt: 3 },
    value: 'props',
    optional: false,
    typeAnnotation: {
      type: 'TsTypeAnnotation',
      span: [Object],
      typeAnnotation: [Object]
    }
  }
}

I'm working in Typescript, which might be the issue? At any rate, param.pat.type wouldn't match. It would be great to hear your thoughts on how best to proceed. I'll push where I'm up to to that branch.

@raph90
Copy link
Contributor Author

raph90 commented Dec 8, 2023

Hi @Josh-Walker-GM , just an FYI I'm still working on this, very close now, so hoping to have a finished PR by tonight.

@raph90
Copy link
Contributor Author

raph90 commented Dec 8, 2023

Hi @Josh-Walker-GM , #9639 is now ready for review. Hopefully all looks good!

@Josh-Walker-GM
Copy link
Collaborator

Thanks @raph90! Again apologies about my sluggish responses, the PR is hugely appreciated! I'll review this as soon as I can.

@raph90
Copy link
Contributor Author

raph90 commented Dec 17, 2023

Hi @Josh-Walker-GM and @Tobbe, I'm trying to run
yarn rw exp studio and I'm getting

Error: Cannot find module 'api/lib/config'

This is while running the project sync with my local redwood fork. Do you have any idea what might be causing this issue?

@Tobbe
Copy link
Member

Tobbe commented Dec 17, 2023

@raph90 Do you get any more info than just that error message? Any line numbers, a stack trace, or anything like that, that might help narrow the error down?

Also some more clear instructions on how to reproduce or exactly what you're doing to see that error could help us help you 🙂

@Josh-Walker-GM
Copy link
Collaborator

@raph90 my gut reaction is that perhaps using a relative path might fix this but I haven't had too much time to think about this

@raph90
Copy link
Contributor Author

raph90 commented Dec 18, 2023

Hi @Josh-Walker-GM and @Tobbe, yeah sorry for the very unspecific issue. I think there must have been another code change that affected our paths - I'm new to open source so just getting used to things 😆

For the paths, I've updated to a relative path. I tried to use the paths key of the tsconfig but for some reason I couldn't get that working.

I've updated this PR now with some new code - @Josh-Walker-GM I've removed the variables as you suggested, and I've now made it clear what the default exports are and what function / variable exports look like.

I've also needed to update the yarnrc.yml to get things working - I'm not sure if we want to add this as a redwood wide thing or if it should be restricted to the studio package, or perhaps if we just want to tell users they need to add it themselves.

But at any rate, this is now working, so hopefully you like what you see. Thanks!

@Tobbe
Copy link
Member

Tobbe commented Dec 18, 2023

That's weird about the yarnrc.yml updates. Unless @Josh-Walker-GM knows what's going on there we might have to bring in our yarn expert @jtoar to take a look

@Josh-Walker-GM
Copy link
Collaborator

That's perfect @raph90. I'll find some time to review this again today or tomorrow. We can have this merged soon for sure!

For some context, the studio package isn't representative of the rest of the framework. I quickly created it as an experimental feature so it wasn't built to the same standard as the rest of our packages. Over the new year we'll be transitioning the studio to its own repository built with redwood itself. This should fix these sorts of configuration issues so I'd feel bad making anyone else sink lots of time into fixing these chores.

Josh-Walker-GM added a commit that referenced this issue Dec 24, 2023
Reference to #9620. 

Currently Studio supports `export function Welcome()` syntax for emails.
This PR adds support for `export const Welcome = () => ...`

Also added optional dependencies for SWC to fix an error that _I think_
is architecture related.

---------

Co-authored-by: Josh GM Walker <56300765+Josh-Walker-GM@users.noreply.github.com>
@Josh-Walker-GM
Copy link
Collaborator

Closed in #9639

dac09 added a commit to dac09/redwood that referenced this issue Dec 27, 2023
…redwood into fix/enhance-error-apollo-suspense

* 'fix/enhance-error-apollo-suspense' of github.com:dac09/redwood: (92 commits)
  chore(deps): update dependency @types/yargs to v17.0.32 (redwoodjs#9759)
  Make it easier to find useMatch docs (redwoodjs#9756)
  chore(unit tests): Use side-effect import to fix TS errors (redwoodjs#9754)
  fix(context): Refactor context (redwoodjs#9371)
  docs: Replaced deprecated <Set private> with PrivateSet within router.md (redwoodjs#9749)
  add TS support for storybook preview tsx config extension (redwoodjs#9309)
  fix(studio): Fix windows path issues (redwoodjs#9752)
  redwoodjs#9620: Update studio to support variable components (Mailer) (redwoodjs#9639)
  chore(tasks): Add comparison view to nmHoisting visualisation (redwoodjs#9751)
  chore(cli): make fs modules used in the CLI consistent (redwoodjs#9746)
  chore(tooling): Make sure console boxen print on a new line
  chore(CI): fix publish release candidate
  feat(CLI): add check node version middleware, rm `.nvmrc`, yarn engines (redwoodjs#9728)
  docs: added some clarification on serverless functions getting executed in a non-serverless environment (redwoodjs#9742)
  Fix sshExec() errors not displaying (redwoodjs#9743)
  chore(tooling): Add missing word in release tooling output
  Update Metadata docs (redwoodjs#9744)
  chore(CI): update test project fixture and CRWA for deploy target CI repo (redwoodjs#9730)
  chore(tooling): add script for getting nested dependency data (redwoodjs#9734)
  Trusted Documents docs: Proofreading corrections (redwoodjs#9737)
  ...
dac09 added a commit to dac09/redwood that referenced this issue Dec 27, 2023
…ath-aliases

* 'main' of github.com:redwoodjs/redwood: (92 commits)
  chore(deps): update dependency @types/yargs to v17.0.32 (redwoodjs#9759)
  Make it easier to find useMatch docs (redwoodjs#9756)
  chore(unit tests): Use side-effect import to fix TS errors (redwoodjs#9754)
  fix(context): Refactor context (redwoodjs#9371)
  docs: Replaced deprecated <Set private> with PrivateSet within router.md (redwoodjs#9749)
  add TS support for storybook preview tsx config extension (redwoodjs#9309)
  fix(studio): Fix windows path issues (redwoodjs#9752)
  redwoodjs#9620: Update studio to support variable components (Mailer) (redwoodjs#9639)
  chore(tasks): Add comparison view to nmHoisting visualisation (redwoodjs#9751)
  chore(cli): make fs modules used in the CLI consistent (redwoodjs#9746)
  chore(tooling): Make sure console boxen print on a new line
  chore(CI): fix publish release candidate
  feat(CLI): add check node version middleware, rm `.nvmrc`, yarn engines (redwoodjs#9728)
  docs: added some clarification on serverless functions getting executed in a non-serverless environment (redwoodjs#9742)
  Fix sshExec() errors not displaying (redwoodjs#9743)
  chore(tooling): Add missing word in release tooling output
  Update Metadata docs (redwoodjs#9744)
  chore(CI): update test project fixture and CRWA for deploy target CI repo (redwoodjs#9730)
  chore(tooling): add script for getting nested dependency data (redwoodjs#9734)
  Trusted Documents docs: Proofreading corrections (redwoodjs#9737)
  ...
Tobbe pushed a commit that referenced this issue Jan 1, 2024
Reference to #9620. 

Currently Studio supports `export function Welcome()` syntax for emails.
This PR adds support for `export const Welcome = () => ...`

Also added optional dependencies for SWC to fix an error that _I think_
is architecture related.

---------

Co-authored-by: Josh GM Walker <56300765+Josh-Walker-GM@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/confirmed We have confirmed this is a bug topic/mailer RedwoodJS Mail and Mailer related
Projects
None yet
Development

No branches or pull requests

3 participants