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

pnpm monorepo support - ppx resolution failing #104

Open
tabazevedo opened this issue Apr 15, 2024 · 3 comments
Open

pnpm monorepo support - ppx resolution failing #104

tabazevedo opened this issue Apr 15, 2024 · 3 comments

Comments

@tabazevedo
Copy link

Following the recent addition of pnpm support I was looking to move our monorepo to rewatch and noticed a couple of issues which I've reproduced in a thin repo.

Firstly, ppx binary resolution isn't working correctly when ppx dependency is from a child.

In this monorepo example (mind the branch!) we have dependencies set up as follows

  • @monorepo/root has no rescript of its own, but is configured with @monorepo/main as a bs-dependency
  • @monorepo/main depends on @monorepo/library
  • @monorepo/library depends on rescript-logger and uses "ppx-flags": ["rescript-logger/ppx"]

Actual outcome, running rewatch build in root:

❯ rewatch build .
[1/7]📦 Building package tree...Could not read folder: test/intl...
[1/7] 📦 Built package tree in 0.00s
[2/7] 🕵️  Found source files in 0.00s
[3/7] 📝 Read compile state 0.01s
[4/7] 🧹 Cleaned 0/92 0.00s
[5/7] 🧱 Parsing... ⠁ 1/1                                                                                                                                                                
err: sh: /Users/tiago/src/tabazevedo/rewatch-pnpm-test/node_modules/rescript-logger/ppx: No such file or directory

  We've found a bug for you!
  /Users/tiago/src/tabazevedo/rewatch-pnpm-test/packages/library/src/Library.res

  Error while running external preprocessor
Command line: /Users/tiago/src/tabazevedo/rewatch-pnpm-test/node_modules/rescript-logger/ppx '/var/folders/fl/k9vmqsxx3yl92r6_ch6rvc5w0000gn/T/ppx6b9ce9Library.res' '/var/folders/fl/k9vmqsxx3yl92r6_ch6rvc5w0000gn/T/ppx74d7b1Library.res'



[5/7] ️🛑 Error parsing source files in 0.01s
sh: /Users/tiago/src/tabazevedo/rewatch-pnpm-test/node_modules/rescript-logger/ppx: No such file or directory

  We've found a bug for you!
  /Users/tiago/src/tabazevedo/rewatch-pnpm-test/packages/library/src/Library.res

  Error while running external preprocessor
Command line: /Users/tiago/src/tabazevedo/rewatch-pnpm-test/node_modules/rescript-logger/ppx '/var/folders/fl/k9vmqsxx3yl92r6_ch6rvc5w0000gn/T/ppx6b9ce9Library.res' '/var/folders/fl/k9vmqsxx3yl92r6_ch6rvc5w0000gn/T/ppx74d7b1Library.res'


  ️🛑 Could not parse Source Files

It's expecting the rescript-logger library to be hoisted to the top-level and looking up the binary there:
PROJECT_ROOT/node_modules/rescript-logger/ppx

Expected outcome

Binary lookup path is non-hoisted variant:
PROJECT_ROOT/node_modules/@monorepo/main/node_modules/@monorepo/library/node_modules/rescript-logger/ppx


Let me know if I'm missing something, I'll raise a couple of other issues with similar findings in other scenarios.

@tsnobip
Copy link

tsnobip commented Apr 17, 2024

I got the same issue using rescript-schema-ppx. Resolution of regular dependencies does seem to be solved by #91 but indeed PPX resolution seems to still be incorrect unfortunately. Don't hesitate to tell us what we can do to help pinpoint or solve the issue.

Great job on all of this btw (+ incremental build), can't wait to switch my monorepo to rewatch!

@rolandpeelen
Copy link
Member

rolandpeelen commented May 7, 2024

Thanks for the extensive explanation. It does indeed look like we're not resolving ppx's properly. We started building this with yarn, it symlinks packages in monorepos through the root node_modules.
I'll try and have a look this week. But if you would want to tinker with it, you could have a look at how we generate the ppx flags in parse.rs. My hunch is we'd need similar logic as in the other PR.

Ie - we'd need to check if it's either the root or the other path.

@jfrolich - Perhaps we can try and determine from the presence of lock files wether we're dealing with Yarn or Pnpm and drop the overhead of this potential lookup?

@tsnobip
Copy link

tsnobip commented May 9, 2024

Thanks for the detailed explanation.

For the records, I have this issue with my yarn modern (v4) monorepo setup.

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