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

Preview server error since 0.7.2 in yarn workspace #163

Closed
willviles opened this issue Sep 2, 2022 · 12 comments
Closed

Preview server error since 0.7.2 in yarn workspace #163

willviles opened this issue Sep 2, 2022 · 12 comments
Assignees

Comments

@willviles
Copy link

willviles commented Sep 2, 2022

Have tried to update to mailing@^0.7.2 and unfortunately I'm getting this error when attempting to run mailing preview.

Server Error
Error: Cannot find module 'http'

This error happened while generating the page. Any console logs will be displayed in the terminal window.
Call Stack
webpackEmptyContext
file:///Users/willviles/Code/@Redbrain/packages/email/.mailing/.next/server/pages/index.js (22:10)
../../node_modules/axios/lib/adapters/http.js
webpack-internal:///./src/moduleManifest.js (1924:20)
__require2
webpack-internal:///./src/moduleManifest.js (34:56)
getDefaultAdapter
webpack-internal:///./src/moduleManifest.js (2240:27)
../../node_modules/axios/lib/defaults/index.js
webpack-internal:///./src/moduleManifest.js (2259:22)
__require2
webpack-internal:///./src/moduleManifest.js (34:56)
../../node_modules/axios/lib/core/transformData.js
webpack-internal:///./src/moduleManifest.js (2338:24)
__require2
webpack-internal:///./src/moduleManifest.js (34:56)
../../node_modules/axios/lib/core/dispatchRequest.js
webpack-internal:///./src/moduleManifest.js (2362:29)
__require2
webpack-internal:///./src/moduleManifest.js (34:56)

There are no errors in a vanilla project, however I'm using Mailing within a yarn workspace, which already contains a next.js app.

Pinning to mailing@0.7.1 and adding a the following nohoist rules to my root package.json ensures the error disappears:

{
  "workspaces": {
    "nohoist": [
      "**/mailing",
      "**/mailing-core"
    ]
  }
}
@psugihara
Copy link
Collaborator

Thanks for filing an issue @willviles and apologies for the breaking change on a patch release.

  1. Could you give me an idea of your project's file structure?
  2. Could you give your mailing config?
  3. Are you doing anything in templates besides rendering react? (e.g. data fetching, etc)

A small repo that reproduces the issue would be incredible, but I know that's a pain to put together so maybe I could try to cobble one together with the above info.

@psugihara
Copy link
Collaborator

Also, you might try upgrading to latest (0.7.4). There was a small problem in the 0.7.2 release (though I don't think it would be related).

@willviles
Copy link
Author

Hi @psugihara, firstly thanks for such an awesome mail library 👍

I've created a minimal repo here for my use case. I'm using Mailing inside an email package, which is then consumed by a number of server apps in my workspace.

The issue arises when I export a helper class for my Sendgrid logic (importing @sendgrid/client which uses axios) from index.ts. I assume this is because anything exported from index.ts will be bundled by the preview Next.js app and there's a conflict with the axios installed by @sendgrid/client.

All of my email logic doesn't necessarily need to be exported from the root module, but I'd ideally like it so that the sendMail function, all email helper functions and all of my templates can be imported from this module's main file, like so:

import { sendMail, Sendgrid, FooEmail, BarEmail } from 'email'

Understand this may be an annoyingly atypical use case of the library, but this does work if mailing is downgraded to <=0.7.1!

@psugihara
Copy link
Collaborator

Got it, thanks so much for the clean repro repo! I'm away from my dev machine today but will try it this week.

  1. I agree that's a nice interface for the emails package (if it worked 😆). Perhaps a workaround could be adding an index file at packages/emails/index.ts that exports sendMail from packages/emails/src/index.ts as well as your other exports. This file would never get touched by mailing so should not run into any conflicts with the preview server.
  2. The big change in 0.7.2 was that we bundle (with esbuild) your emailsDir (src in this case). We do this so that if you import source files (e.g. a shared colors.ts) from outside of emailsDir, they'll get sucked into the bundle (otherwise they'd just error). Off-hand I can't think why that would break your case. Maybe @alexfarrill who implemented that piece would know.

@alexfarrill
Copy link
Collaborator

Thanks for the repro repo :) I just had a moment to boot it and inspect the emitted moduleManifest.ts. the first thing I notice is that it is bigger than it should be (~100KB vs. ~10KB) and the http part it looks like is being bundled although it should be considered an external dependency and not bundled. i.e. it looks like it is bundling all external dependencies.

The line that indicates this to esbuild is the external setting here:

esbuild docs: https://esbuild.github.io/api/#external

and looking at that, I can see why - because it is looking at the wrong package.json to discover dependencies to exclude. i.e. this is a side effect of the folder structure here -- is this a common setup for turbo/monorepos?

I'm pretty sure that is the cause. i think we might want to read the package.json that's in .mailing like we do here instead of with in import statement. let me see if that would work, or let me know if you have another idea

@alexfarrill
Copy link
Collaborator

We're also not excluding devDependencies

@alexfarrill
Copy link
Collaborator

mm, I think it is actually to do with the fact that @sendgrid/client includes axios, but axios is not declared as a dependency. instead of looking at package.json for what packages are available in the environment, we may have to ask yarn or npm for a list?

@alexfarrill
Copy link
Collaborator

seems related: evanw/esbuild#619

@alexfarrill
Copy link
Collaborator

@willviles @psugihara think I have a solution here, will post a beta to test here soon

@alexfarrill
Copy link
Collaborator

Hi @willviles this is fixed in 0.7.5-next.0
I've tried it out in your test repo. If you want to give it a try as well in your project and let us know if it works, I'd appreciate it!

@alexfarrill alexfarrill self-assigned this Sep 8, 2022
alexfarrill added a commit that referenced this issue Sep 9, 2022
@willviles
Copy link
Author

Wow, you guys are fast!

Can confirm that 0.7.5-next.0 is working perfectly in my real world monorepo.

Thank you @alexfarrill @psugihara for such a speedy fix 👍

@alexfarrill
Copy link
Collaborator

alexfarrill commented Sep 9, 2022 via email

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

No branches or pull requests

3 participants