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

Sourcemaps for .re (Reason) files #716

Closed
jchavarri opened this issue Nov 20, 2018 · 9 comments
Closed

Sourcemaps for .re (Reason) files #716

jchavarri opened this issue Nov 20, 2018 · 9 comments
Labels

Comments

@jchavarri
Copy link
Contributor

I'm setting up a small project to learn about js_of_ocaml with incr_dom, and using the Reason syntax (https://github.com/jchavarri/incr_dom_reason/).

Everything works 🎉 but the sourcemaps for Reason files seem to be broken:

sourcemaps

They seem to point to the .re.ml files that are generated on the build folder.

Is there any configuration that I can use to fix this? cc @jordwalke

@jordwalke
Copy link

Last time I checked the source maps (even for .ml) need a little more work to be generally usable. But for this particular bug, I'm surprised it's happening. I saw .re sourcemaps working somewhat correctly a long time ago but that was before Dune was the one generating them. The issue is that Dune first generates a .ml file (which is just a raw serialized AST that happens to have an .ml extension) to feed to ocamlc. Normally you'd just supply the .re file as input directly to ocamlc. Even so, I'm surprised because I would think that the locations embeded in the output .ml serialized AST are actually pointing to the .re file, and those locations are where the the source maps are generated from so I'd think it would be correct. You could probably put a hack in jsoo in the source maps generation that just knows to redirect all '.re.ml' to .re.

@jchavarri
Copy link
Contributor Author

@jordwalke Thanks. I confirm I have tested the same project but using .ml files with the existing config, and I can see sourcemaps being generated fine (I wanted to be sure I wasn't somehow using a wrong config).

I'll investigate the fix you mention.

@hhugo hhugo added the bug label Nov 25, 2018
@jchavarri
Copy link
Contributor Author

Update: I started a simpler project that includes just the bare configuration of jsoo, dune and esy, and source maps in that case work beautifully:

sourcemaps

So it seems to be an isolated issue of the original repo I mentioned in the description. That repo includes a bunch of ppxs, not sure if it might be related.

@jordwalke
Copy link

Very interesting! yeah, I wonder if the ppx's are somehow dropping the location information or producing the wrong format of intermediate file.

But I noticed your simpler project has a ppx (preprocess (pps js_of_ocaml-ppx)) and that seems to be working fine?

@hhugo
Copy link
Member

hhugo commented Dec 2, 2018

I believe this is an issue with how dune handles some reason files.

Compilation steps go as follow:

  1. parse the source name.re with refmt and output name.re.ml (marshaled ocaml ast with correct locations)
  2. apply some ppx on name.re.ml and output name.re.pp.ml (locations are now wrong and refer to name.re.ml)

Passing args -loc-filename src/name.re when applying ppx seems to fix the issue.

cc @rgrinberg, @diml

@jordwalke
Copy link

Great find. I imagine that would also end up causing other issues too.

@ghost
Copy link

ghost commented Dec 3, 2018

Hmm, actually maybe ppxlib is trying to be too clever here. When the filename read from the marshaled ast file doesn't match what ppxlib thinks the filename is, it rewrites the AST with the new name. At least, we shouldn't rewrite the AST unless -loc-filename is passed explicitly.

@hhugo
Copy link
Member

hhugo commented Dec 9, 2018

Ppxlib has been fixed. Closing.

@hhugo hhugo closed this as completed Dec 9, 2018
@jordwalke
Copy link

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants