reasonml / reason-react

Reason bindings for ReactJS
https://reasonml.github.io/reason-react/
MIT License
3.25k stars 347 forks source link

Why does the ppx have a separate opam file? #827

Closed rikimasan closed 8 months ago

rikimasan commented 8 months ago

This ends up creating multiple copies of the same code locally which is causing issues with jest haste map finding two instances of the same thing. Found while using the metro bundler for a react native project. Seems like this could all be under one opam file and just have the ppx be reason-react.ppx especially since you don't want the ppx version to mismatch the reason-react version.

rikimasan commented 8 months ago

We were able to fix this for now by adding the following to our metro.config.js, however I'm not sure this is the ideal solution? I don't think metro should need to know about the opam folder so it's probably fine but I would still like some insight as to why the choice to have them separate was taken.

const config = {
    // ...
    resolver: {
      blacklistRE: blacklist(
        [
          /.*\/\_opam\/.*/,
        ].concat(
          workspaces.map(
            (workspacePath) =>
              `/${workspacePath.replace(/\//g, '[/\\\\]')}[/\\\\]node_modules[/\\\\]react-native[/\\\\].*/`
          ))
      ),
      // ...
    },
  };
anmonteiro commented 8 months ago

Why does the ppx have a separate opam file?

Because it's a different package than the reason-react library. It could even be used with other libraries as long as they remain compatible with the generated PPX API.

As for the _opam folder, you're right that metro shouldn't need to know about it. If you're using Melange with Dune + OPAM, you'll get all the JS files in the melange.emit target directory -- those are all the artifacts that your bundler should care about.

davesnx commented 8 months ago

Hey @rikimasan, can you share a minimised example where metro reads stuff from _opam?

I would like to understand why a separate opam package causes an issue here.

Thanks. (I will keep the issue closed since there's non issue in reason-react, but happy to move this further)