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.

Sign up for GitHub

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

Jump to bottom

playground: reason syntax and react ppx #602

Merged
merged 47 commits into from
Jun 21, 2023
Merged

playground: reason syntax and react ppx #602

merged 47 commits into from
Jun 21, 2023

Conversation

jchavarri
Copy link
Member

@jchavarri jchavarri commented Jun 2, 2023 •
edited
Loading

Based on #538, continues the work from there to add:

  • Integration with reason syntax
  • reactjs ppx
  • removes special handling for stdlib, so packages like melange.belt are required as if they were in node_modules
  • investigate issue with reason parser choking on // input
  • issue with errors without locations (playground: reason syntax and react ppx #602 (comment))

jchavarri added 20 commits April 7, 2023 16:37
@jchavarri
playground: make jsoo_main compile
0653696
@jchavarri
Merge branch 'main' into playground-1
bea1835
@jchavarri
playground: updates after merging main
6944289
@jchavarri
Merge branch 'main' into playground-1
eb98362
@jchavarri
playground: use private melange comp libs
0809424
@jchavarri
playground: pick up change in translmod
9cbe9c5
@jchavarri
playground: better error handling
ec1fbe3
@jchavarri
playground: remove hardcoded BS_BROWSER
ba38c05
@jchavarri
opam: add jsoo
ca209c4
@jchavarri
playground: update in-code comment
d1ce4ed
@jchavarri
playground: add new profile browser
01c797b
@jchavarri
playground: build in ci
4e64b6f
@jchavarri
Revert "playground: pick up change in translmod"
b178930 
This reverts commit 9cbe9c5.
@jchavarri
Merge branch 'main' into playground-1
0137a6b 
* main:
  test(rescript-syntax): fix rescript-syntax test (#592)
  fix: restore the old error message
  melange ppx: fatal error when parsing %raw with cb
  chore: update melange-compiler-libs (#591)
  GA(deps): bump cachix/install-nix-action from 20 to 21 (#589)
@jchavarri
playground: add dev mode for faster builds
b9a2554
@jchavarri
playground: apply ppxlib driver on structure
08be825
@jchavarri
playground: add comment
b23d4f4
@jchavarri
Merge branch 'main' into playground-1
5ef2d1f 
* main:
  chore: use default dune modes for executables (#596)
  refactor(ci): use an explicit install phase for the nix tests (#597)
  chore: move reactjs-jsx-ppx to reasonml/reason-react (#595)
  chore: remove dune pins (#593)
@jchavarri
add parse/print funs, run reactjs ppx
42b51ef
@jchavarri
dump without /./
24d0f07
@jchavarri
Copy link
Member Author

jchavarri commented Jun 2, 2023 •
edited by anmonteiro
Loading

@anmonteiro I am running into some issue with errors from ppxlib, where the locations are missing for some reason.

Repro:

$ make dev
$ find _build/default/jscomp/stdlib/.stdlib.objs/melange -name "*.cmi" -or -name "*.cmj" | xargs js_of_ocaml build-fs -o stdlib.js
$ make playground-dev
$ node
Welcome to Node.js v18.16.0.
Type ".help" for more information.
> require('./_build/default/bin/jsoo_main.bc.js'); require('./stdlib.js');
{}
> ocaml.compile("let t = [%bs.obj 2]");
{ js_error_msg: 'Expect a record expression here' }

The problem seems to be that Location.error_of_exn returns None for this error, do you have any clues? I tried removing the code added in #590, but nothing changed.

anmonteiro
anmonteiro reviewed Jun 5, 2023
View reviewed changes
bin/jsoo_common.ml
@@ -86,7 +85,11 @@ module Reason = struct
(* you can't throw an Error here. jsoo parses the string and turns it
into something else *)
let throwAnything = Js.Unsafe.js_expr "function(a) {throw a}" in
try code |> Js.to_string |> Lexing.from_string |> f
let code =
(* Add ending new line as otherwise reason parser chokes with inputs such as "//" *)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this reasonml/reason#2350, which was fixed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like. I have no idea why it's reappearing now, maybe because in playground case we don't have EOF?

Base automatically changed from playground-1 to main June 5, 2023 23:43
@jchavarri jchavarri marked this pull request as ready for review June 6, 2023 13:27
@jchavarri jchavarri requested a review from anmonteiro June 6, 2023 13:29
@jchavarri
Copy link
Member Author

jchavarri commented Jun 6, 2023 •
edited
Loading

This is ready, the only thing I could not figure out is why when there are errors coming from melange ppx, the line locations are gone (asked above).

@jchavarri jchavarri mentioned this pull request Jun 7, 2023
Merged
@anmonteiro
Copy link
Member

mystery solved 1b89c2f

anmonteiro
anmonteiro reviewed Jun 17, 2023
View reviewed changes
Copy link
Member

@anmonteiro anmonteiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to depend on Reason 3.9 before we merge this (see ocaml/opam-repository#23945).

That release ports Reason to ppxlib and breaks the current interface file signature.

@jchavarri
Copy link
Member Author

mystery solved 1b89c2f

Thanks!

I'd like to depend on Reason 3.9 before we merge this (see ocaml/opam-repository#23945).

Sounds good.

anmonteiro
anmonteiro approved these changes Jun 21, 2023
View reviewed changes
bin/jsoo_main.ml
let make_compiler name impl =
export name
let () =
export "ocaml"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps we should call this melange.

feel free to do in a future PR, I don't want to unblock this one further

@anmonteiro anmonteiro merged commit a8f6333 into main Jun 21, 2023
@anmonteiro anmonteiro deleted the playground-2 branch June 21, 2023 05:33
@jchavarri jchavarri mentioned this pull request Jun 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anmonteiro anmonteiro anmonteiro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jchavarri @anmonteiro