pnpm monorepo support - ppx resolution failing

tabazevedo commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

Thanks for the detailed explanation.

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

rescript-lang / rewatch

pnpm monorepo support - ppx resolution failing #104