Move React ppx from compiler repo, add tests #124

jchavarri · 2020-08-30T17:51:08Z

I find myself wanting to contribute to React ppx but always end up giving up because of the lack of infrastructure. By moving the file here, we can leverage the testing infrastructure and keeping it more in sync with the parser.

If this makes sense and gets merged, I will update the compiler repo to read from new file.

Hopefully this change makes sense as well from organizational perspective, as the React ppx is a syntax transformation.

Summary of changes:

Copied the React ppx file from:
https://github.com/rescript-lang/rescript-compiler/blob/4f4812aa4e1411029392f6dbcacc580e755d68d5/jscomp/syntax/reactjs_jsx_ppx.cppo.ml, and then applied these changes:
- Add @raise annotations
- Remove dead functions
- Removed macro #ifdef BINARY
- Remove unused Ast_mapper.register call
- Format file using width 120
Add reactjs_jsx_ppx.mli file
Added -reactJsx option to rescript.exe (for now I didn't added it to command -help output as I'm not sure if it should be exposed)
Adapt test runner to interpret test files .react.res and react.resi
Add a few tests

Thanks to these changes, I already found 2 bugs:

Comments at the top of the file are placed in the wrong position
<Foo.bar /> elements are not supported by new parser

React ppx copied from: https://github.com/rescript-lang/rescript-compiler/blob/4f4812aa4e1411029392f6dbcacc580e755d68d5/jscomp/syntax/reactjs_jsx_ppx.cppo.ml

tests/runner.js

IwanKaramazow · 2020-08-30T18:11:18Z

@jchavarri This is great! Will take a look at your PR. Long term we want to merge the jsx ppx with the parser itself, i.e. do the syntax transform in the parser.

jchavarri · 2020-08-30T19:55:54Z

Thanks for the responsiveness @IwanKaramazow! Much appreciated :)

Long term we want to merge the jsx ppx with the parser itself.

I realized while working on this that there is no need to have it as a separate step now. It makes sense to do so, although maybe in mid-long term? e.g. 6-12 months. I mention that because I guess that merging the ppx with the parser would potentially break conversion between code in Reason and ReScript syntaxes? 🤔 Which is really useful while this transitions happens.

Also, I don't know if it's possible, but it'd be great to keep it as a separate module, so that tools like https://jchavarri.github.io/ppx-explorer/ that rely on a PPX-like API can keep updates with upstream easily, e.g here.

chenglou · 2020-08-31T07:31:35Z

Hey Javier! It's been a while
We're gonna pause this specifically, because the React PPX might change in the future (cc @rickyvetter). Switching version without breakage would require a big syntax-level switch, which would need some consideration.

jchavarri · 2020-08-31T08:46:10Z

@chenglou hey! long time indeed :)

Could you elaborate, please? This PR doesn't change the ppx semantics, and it does not contain any breaking changes in the ppx code. All changes in Reactjs_jsx_ppx module are purely cosmetic (formatting, add reanalyze @raise attributes etc).

If there is some breaking change planned for the future for the ppx, this PR should actually help @rickyvetter and any other authors build and release that change, because once there are more tests, we will know exactly what cases are breaking after the change.

In other words, if this PR gets merged, nobody should notice anything, except the ppx maintainers and contributors, who would get a quality of life improvement thanks to the ppx now being part of the testing infrastructure in this repo.

jchavarri · 2020-08-31T09:03:02Z

Oh, I think I see what you mean. If PPX lives in compiler repo, then it's easy to configure bsconfig flags like it was done with jsx2 and 3. 😞 I didn't think of that.

Maybe the parser could offer a way to switch between syntaxes per file with an attribute, like Reason parser is doing: https://github.com/facebook/reason/pull/2605/files#diff-2f961a549953a72af3a11bb3b36ecc49R4975.

This would be helpful not only for ppx of course, but any breaking syntax change.

jchavarri · 2020-08-31T10:30:25Z

@chenglou I thought more about this. Because syntax is consumed as a git submodule inside compiler repo, nothing would prevent having 2 ppx modules with 2 different versions, and then adding any bsconfig configuration in compiler repo like it's been done in the past. From the compiler perspective, nothing changes either after this change, except it has to read the ppx file from another location. I'm curious to know what @bobzhang thinks as well.

bobzhang · 2020-08-31T11:54:25Z

@jchavarri Ricky (@rickyvetter) owns the module.
There's a concern since we still need support the reason syntax, so if we spins off the jsx ppx, how would it affect the old reason syntax

jchavarri · 2020-08-31T13:03:31Z

@bobzhang There should be no impact in old Reason syntax. From the compiler perspective, this PR can be considered for all effects and purposes like moving a file from one folder to another.

bobzhang · 2020-08-31T13:12:48Z

As long as it does not make the main maintainer’s life more difficult, I am fine with it. But keep in mind this module is used by reason syntax too Javier Chávarri <[email protected]>于2020年8月31日周一下午9:03写道：

@bobzhang <https://github.com/bobzhang> There should be no impact in old Reason syntax. From the compiler perspective, this PR can be considered for all effects and purposes like moving a file from one folder to another. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#124 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFWMK52VPL4VL46JZEICSDSDONTHANCNFSM4QPX2FPQ> . --

Regards -- Hongbo Zhang

jchavarri · 2020-08-31T19:49:23Z

I created a draft PR to show how minimal the impact downstream for the compiler is: rescript-lang/rescript#4660. Hopefully that and the comments above help unblock the PR 🙏

rickyvetter · 2020-09-02T15:37:40Z

I think this move could make maintaining quite a bit cleaner but I want to clarify setup. In the near future we need to make a series of major breaking changes to the ppx and need to ship both versions to the compiler so that folks can update their ppx version independently of their compiler version. Would you propose just exposing both versions as different modules here and then doing the switch logic in the compiler based on config?

jchavarri · 2020-09-02T20:27:16Z

Hey @rickyvetter ! 👋

Would you propose just exposing both versions as different modules here and then doing the switch logic in the compiler based on config?

Yes, exactly. The companion PR in the compiler repo shows how the ppx module is available just like before (see ppx_entry module). I only had to change the linking order so that napkin.cmxa lib (this repo) is linked before the syntax.cmxa lib (that contains the ppx entry in compiler), which depends on it.

If there is a new version of the ppx, there can be another module added next to the one in this PR. Keeping these modules close to the testing infra might be beneficial too to guarantee the migration is smooth, and new tests can also be added from user reports. It seems the compiler still keeps the ppx version handling from bsconfig option.

The only thing that will change from previous setup is that every time there is a fix or update in the ppx, the compiler repo will have to sync the submodule version, while before the file could be just directly updated in-repo. I hope this is not a deal breaker.

Maybe I'm missing something :) Please let me know if there's anything else that I'm not accounting for 👍

rickyvetter · 2020-09-03T19:19:45Z

Gotcha. Seems like a win to me. @chenglou you seemed hesitant at first - okay with this plan?

jchavarri · 2020-09-21T22:40:38Z

updated the PR with the following:

merge the changes from master
add flag -ppx to the main rescript.exe binary (as discussed with @IwanKaramazow offline)
remove ppx-related stuff from multiprinter (as jsx ppx seems very unrelated to printing really)
skip roundtrip tests for specs that have ppx defined
added some test externalWithCustomName that was failing before but now passes, I guess some recent change fixed it.

lmk what you think :)

IwanKaramazow

Left a couple of questions, what do you think?

IwanKaramazow · 2020-09-22T05:47:12Z

src/res_cli.ml

@@ -243,8 +251,12 @@ module CliArgProcessor = struct
          else exit 1
        end
        else
+          let parsetree = match ppx with


Can we keep this a string and match on the string for now? Does this extra conversion give us something in the end?

IwanKaramazow · 2020-09-22T05:48:27Z

Makefile

@@ -49,6 +50,8 @@ lib/rescript.exe: $(CLI_FILES)

 build-native: lib/refmt.exe lib/rescript.exe depend

+build-ppx: src/reactjs_jsx_ppx.cmx


why does the ppx need a separate build-ppx rule? If it's a part "API_FILES", is this sufficient?

IwanKaramazow · 2020-09-22T05:49:06Z

src/res_cli.ml

@@ -178,6 +179,7 @@ end = struct
  let origin = ref ""
  let interface = ref false
  let report = ref "pretty"
+  let ppx = ref "none"


can we default this to the empty string ""?

IwanKaramazow · 2020-09-22T05:49:35Z

src/res_cli.ml

@@ -227,6 +230,11 @@ module CliArgProcessor = struct
      | _ -> false
      in

+      let ppx = match ppx with


See comment down, this conversion doesn't add that much value?

IwanKaramazow · 2020-09-22T05:50:55Z

tests/runner.js

@@ -204,15 +202,16 @@ let makeReproducibleFilename = (txt) => {
  })
 };

-global.runPrinter = (dirname) => {
+global.runPrinter = (dirname, ppx = "none") => {


Can we make this the empty string or just keep it undefined as default value?

IwanKaramazow · 2020-09-22T05:51:19Z

tests/runner.js

@@ -226,7 +225,7 @@ Make sure the test input is syntactically valid.`;
        expect(result).toMatchSnapshot();
      }

-      if (process.env.ROUNDTRIP_TEST) {
+      if (process.env.ROUNDTRIP_TEST && ppx === "none") {


Can you add a comment here on why we don't run ppx'es in the roundtrip tests?

IwanKaramazow · 2020-09-22T05:52:25Z

tests/printer/reactJsx/__snapshots__/render.spec.js.snap

@@ -0,0 +1,101 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP


I wonder if we should make a separate folder for ppx tests: tests/ppx/...? What do you think?

IwanKaramazow · 2020-09-22T05:52:40Z

src/res_cli.ml

    with
    | Failure txt ->
      prerr_string txt;
      prerr_newline();
      exit 1
    | _ -> exit 1
-  [@@raises exit]
+  [@@raises Invalid_argument, exit]


What raises Invalid_argument?

The existing react ppx uses that exception to note cases with unexpected situations. I can count at least 19 different places, e.g.

syntax/src/reactjs_jsx_ppx.ml

Lines 351 to 354 in 23336db

raise

(Invalid_argument

"Key cannot be accessed inside of a component. Don't worry - you can always key a component from its \

parent!")

…anup

jchavarri · 2020-09-22T21:13:16Z

@IwanKaramazow I think I tackled all the comments, let me know if there's anything missing :)

IwanKaramazow

@jchavarri Looking good, thank you for all the work.

bobzhang · 2020-10-23T02:13:56Z

This is not finished since the removal of jsx ppx is not done in the compiler.

After some thoughts, I think this may make your test workflow more difficult, @rickyvetter , since now you make changes to the ppx, you need update the syntax submodule to test your new changes

IwanKaramazow · 2020-10-23T04:54:01Z

@bobzhang I'll take care of it.

jchavarri · 2020-10-23T06:34:33Z

@bobzhang I had created a companion PR in rescript-lang/rescript#4660 that included the compiler changes. I didn't continue working on this because @chenglou mentioned on DM a few days after this PR was merged that the move to the PPX to the parser might be rethought / retracted because of the new jsx transform.

IwanKaramazow · 2020-10-23T06:54:25Z

Let's sync up on this first.

jchavarri · 2020-10-23T10:47:14Z

I talked to @IwanKaramazow and these are the steps to take before this is finished:

Update syntax repo so that Reactjs_jsx_ppx becomes Reactjs_jsx_ppx_v3 again, as it will be useful later on, when new JSX ppx versions will be added.
Update Adapt ninja config to use react ppx from upstream syntax submodule rescript#4660 with master and latest syntax, and make sure everything compiles.

Move React ppx from compiler repo, add tests

523ff1e

React ppx copied from: https://github.com/rescript-lang/rescript-compiler/blob/4f4812aa4e1411029392f6dbcacc580e755d68d5/jscomp/syntax/reactjs_jsx_ppx.cppo.ml

jchavarri commented Aug 30, 2020

View reviewed changes

tests/runner.js Outdated Show resolved Hide resolved

Fix roundtrip tests, ignore test files with .fixme suffix

d24c0df

jchavarri mentioned this pull request Aug 30, 2020

Parse <Foo.bar /> elements #125

Merged

jchavarri mentioned this pull request Aug 31, 2020

Adapt ninja config to use react ppx from upstream syntax submodule rescript-lang/rescript#4660

Merged

jchavarri mentioned this pull request Sep 20, 2020

Update parser + test runner to fail when in non-recover mode #146

Merged

jchavarri added 6 commits September 21, 2020 23:53

Merge branch 'master' into react-ppx

a3f0b44

add -ppx cmd line arg

8a7a1bd

rename .react.res to just .res

d3dcd85

remove unneeded jsx stuff from res_multi_printer

dd2f680

remove .fixme option and add previously failing test + snapshot

e84640a

don't run roundtrip tests if ppx is defined

739a1df

IwanKaramazow reviewed Sep 22, 2020

View reviewed changes

move ppx tests to own folder, remove "none" as default arg value, cle…

23336db

…anup

jchavarri requested a review from IwanKaramazow September 22, 2020 21:13

IwanKaramazow approved these changes Sep 24, 2020

View reviewed changes

IwanKaramazow merged commit 9d2be71 into rescript-lang:master Sep 26, 2020

jchavarri deleted the react-ppx branch September 26, 2020 10:12

jchavarri mentioned this pull request Oct 23, 2020

Rename React ppx module to include v3 suffix #159

Merged

chenglou mentioned this pull request Mar 8, 2021

Codepaths to remove in the future #319

Closed

		@@ -49,6 +50,8 @@ lib/rescript.exe: $(CLI_FILES)

		build-native: lib/refmt.exe lib/rescript.exe depend

		build-ppx: src/reactjs_jsx_ppx.cmx

	raise
	(Invalid_argument
	"Key cannot be accessed inside of a component. Don't worry - you can always key a component from its \
	parent!")

Move React ppx from compiler repo, add tests #124

Move React ppx from compiler repo, add tests #124

Uh oh!

Conversation

jchavarri commented Aug 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

IwanKaramazow commented Aug 30, 2020

Uh oh!

jchavarri commented Aug 30, 2020

Uh oh!

chenglou commented Aug 31, 2020

Uh oh!

jchavarri commented Aug 31, 2020

Uh oh!

jchavarri commented Aug 31, 2020

Uh oh!

jchavarri commented Aug 31, 2020

Uh oh!

bobzhang commented Aug 31, 2020

Uh oh!

jchavarri commented Aug 31, 2020

Uh oh!

bobzhang commented Aug 31, 2020 via email

Uh oh!

jchavarri commented Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rickyvetter commented Sep 2, 2020

Uh oh!

jchavarri commented Sep 2, 2020

Uh oh!

rickyvetter commented Sep 3, 2020

Uh oh!

jchavarri commented Sep 21, 2020

Uh oh!

IwanKaramazow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jchavarri Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jchavarri commented Sep 22, 2020

Uh oh!

IwanKaramazow left a comment

Choose a reason for hiding this comment

Uh oh!

bobzhang commented Oct 23, 2020

Uh oh!

IwanKaramazow commented Oct 23, 2020

Uh oh!

jchavarri commented Oct 23, 2020

Uh oh!

IwanKaramazow commented Oct 23, 2020

Uh oh!

jchavarri commented Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jchavarri commented Aug 30, 2020 •

edited

Loading

jchavarri commented Aug 31, 2020 •

edited

Loading

jchavarri Sep 22, 2020 •

edited

Loading

jchavarri commented Oct 23, 2020 •

edited

Loading