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

ppx: set display name on let%component #166

Merged
merged 7 commits into from
May 10, 2022
Merged

ppx: set display name on let%component #166

merged 7 commits into from
May 10, 2022

Conversation

glennsl
Copy link
Contributor

@glennsl glennsl commented Apr 14, 2022

Not sure how to test it, but this produces react stack traces such as this:

The above error occurred in the <Dune__exe__HelloWorldOCaml.Foo.make.(fun)> component:
    in Dune__exe__HelloWorldOCaml.Foo.make.(fun)
    in div (created by Dune__exe__Main.make.(fun))
    in div (created by Dune__exe__Main.make.(fun))
    in div (created by Dune__exe__Main.make.(fun))
    in div (created by Dune__exe__Main.make.(fun))
    in Dune__exe__Main.make.(fun)

Also note that the lower bound on OCaml was bumped to 4.12 to be able to use __FUNCTION__

@glennsl
Copy link
Contributor Author

glennsl commented Apr 14, 2022

CI errors look like flukes to me 🤷

@glennsl
Copy link
Contributor Author

glennsl commented Apr 14, 2022 •
edited

MacOS failure was indeed a fluke. Windows failure seems to be an issue with setup-ocaml: ocaml/setup-ocaml#479

jchavarri
jchavarri approved these changes Apr 15, 2022
View reviewed changes
Copy link
Collaborator

@jchavarri jchavarri left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for this!

I don't see any blockers to merge this, just have a question.

Makefile
@@ -49,4 +49,4 @@ $(opam_file): dune-project ## Update the package dependencies when new deps are

init: ## Create a local opam switch and setups githooks
git config core.hooksPath .githooks
opam switch create . 4.10.0 --deps-only --with-test
opam switch create . --deps-only --with-test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not updating version here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just seems unnecessary. I also think not specifying it as an invariant might make upgrading easier, since then you shouldn't need to reinstall the whole switch. But I haven't actually tried that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking if there is no version explicitly mentioned in make init command, one would install 4.14 (or whatever is latest) just to switch back to 4.12 or whatever we are using for development 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, on second thought, what would happen is that we'd develop with latest version of the compiler, while ci is tied to 4.12. Maybe we should leave it like that (to potentially detect issues in development asap), and enable multi-version ci in the future.

In any case, none of this is blocker for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually installed 4.13.1 for me, for some reason. We could probably add a constraint on dev or with-test to have more precise control over this as well.

ppx/test/pp_ocaml.expected Outdated
@@ -41,7 +41,9 @@ let make =
(Props : < .. > Js_of_ocaml.Js.t) (fun x -> x#name)) in
fun ?name ->
fun ?key ->
fun () -> React.create_element make (make_props ?key ?name ()))
fun () ->
React.set_display_name make __FUNCTION__;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if there is a way to call set_display_name from outside this function, to avoid wasted work. Or is it necessary to do so every time a new element is created?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It most likely could, but I'm struggling to figure out where that would be in the ppx code. We could probably also mitigate the impact by only emitting this in dev mode, but I'm not sure how to do that either. Yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in d0cc8c3

@jchavarri
Copy link
Collaborator

@glennsl this PR would close #79, right?

@glennsl
Copy link
Contributor Author

glennsl commented Apr 16, 2022

@glennsl this PR would close #79, right?

Seems so! Need to figure out how to detect if we're in dev mode though.

@glennsl
Copy link
Contributor Author

glennsl commented Apr 16, 2022

Now it'll only set the display name if the environment variable JSOO_REACT_DEV set.

@jchavarri
Copy link
Collaborator

Thanks!

@jchavarri jchavarri merged commit 8c10acc into ml-in-barcelona:main May 10, 2022
@glennsl glennsl deleted the feat/ppx/display-name branch May 10, 2022 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jchavarri jchavarri jchavarri 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.

None yet
2 participants
@glennsl @jchavarri