Skip to content

Convert test sources from .re to .res #5678

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

Merged
merged 6 commits into from
Sep 21, 2022

Conversation

cknitt
Copy link
Member

@cknitt cknitt commented Sep 20, 2022

  • I had to comment out some code from the old ReasonReact sources in the tests because of Pexp_object usage.
  • There were some changes in doc comments due to the conversion.

Tests in .ml syntax were not converted to .res yet.

@cknitt cknitt marked this pull request as draft September 20, 2022 18:26
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

The conversion looks good.
Left a bunch of comments about stale tests or tests with unclear utility. How about creating a separate issue as a reminder for these?

@@ -0,0 +1 @@
Js.log("Hello, ReScript!")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Separate question: should we drop templates?
They're clearly several years stale, and probably add only negative value at this point.

Copy link
Member Author

Choose a reason for hiding this comment

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

The documentation suggests this: https://rescript-lang.org/docs/manual/latest/installation#new-project
and does not even mention the templates anymore.
So I'd say yes, let's remove them.

}];
*/
@react.component
let rec make = (~foo, ()) => React.createElement(make, makeProps(~foo, ()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Separate question: this is not really testing the ppx. So not clear what this is testing here.

CC @mattdamon108 what kinds of JSX tests should be have here instead?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for being late.
I think we can put the js output test here. The JSX can be presumably tested by the js output either. In order to do that, some of the rescript-react modules need to be put here.

Copy link
Member

@mununki mununki Sep 22, 2022

Choose a reason for hiding this comment

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

Or maybe enough with tests in the syntax?

Copy link
Collaborator

@cristianoc cristianoc Sep 22, 2022

Choose a reason for hiding this comment

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

The syntax does not check that it type checks, or that the generated code does not change. So just adding a few .res files here (and checking in the output) should be enough for the purpose of JSX.

Copy link
Member

Choose a reason for hiding this comment

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

Ah! Correct. The tests are going to be type checked. It seems worthy. I'll put some needful tests here.

external setDisplayName: (component<'props>, string) => unit = "displayName"

@get @return(nullable)
external displayName: component<'props> => option<string> = "displayName"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Separate question: this is probably stale?
Also, what is it testing?


@val
external unsafeAddStyle: (@as(json`{}`) _, style, {..}) => style = "Object.assign"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Strange to have such a large file in tests which are perf critical.


@module("react") external fragment: 'a = "Fragment"

module Router = ReasonReactRouter
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably stale too?


let createClass =
factory(. reactComponent, reactIsValidElement, reactNoopUpdateQueue);
let createClass = factory(. reactComponent, reactIsValidElement, reactNoopUpdateQueue)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is definitely very stale.

@cknitt cknitt marked this pull request as ready for review September 21, 2022 06:36
@cknitt cknitt merged commit 427fb8a into rescript-lang:master Sep 21, 2022
@cknitt cknitt deleted the build-tests-res branch September 21, 2022 06:37
@cknitt cknitt mentioned this pull request Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants