Add rescript example (#442)

[Yassa-hue]

• Add rescript build configurations

• Create rescript page components

• Add rescript deps

---

This change is 

[Yassa-hue]

This PR is not ready yet. I need some help from @alex35mil to fix some issues.

[alex35mil]

Reviewed 24 of 29 files at r1, 10 of 10 files at r2, all commit messages.
Reviewable status: all files reviewed, 32 unresolved discussions (waiting on @Yassa-hue)

---

.gitignore line 51 at r2 (raw file):

.yarn-integrity

lib/bs

Ignore the whole lib as there might be other directories in there, such as ocaml.

---

bsconfig.json line 15 at r2 (raw file):

    }
  ],
  "bsc-flags": [

Add -open Belt.

---

bsconfig.json line 16 at r2 (raw file):

  ],
  "bsc-flags": [
    "-bs-super-errors",

You can remove this one.

---

bsconfig.json line 27 at r2 (raw file):

  ],
  "jsx": {
  "version": 4,

Format.

---

bsconfig.json line 30 at r2 (raw file):

    "mode": "automatic"
  }
}

Configure your editor to add new lines at the end of files on save.

---

client/app/bundles/comments/src/RescriptPage.res line 5 at r2 (raw file):

  let (comments, setComments) = React.useState(_ => ([] : Actions.Api.comments));
  let (error, setError) = React.useState(_ => false)
  let (isSaving, setIsSaving) = React.useState(_ => false)

Refactor this: represent the state as a variant to eliminate the possibility of an invalid state and use reducer to manage the state.

---

client/app/bundles/comments/src/RescriptPage.res line 9 at r2 (raw file):

  let storeComment = (author, text) => {
    setIsSaving(_ => true)
    let _ = (async() => {

You don't need let _ = ... if the return value is unit.

---

client/app/bundles/comments/src/RescriptPage.bs.mjs line 1 at r2 (raw file):

// Generated by ReScript, PLEASE EDIT WITH CARE

We usually gitignore this.

---

client/app/bundles/comments/src/Actions/Actions.res line 8 at r2 (raw file):

  default: rorDefault
}
@module("react-on-rails") external ror: rorModule = "default";

We have these bindings: https://github.com/shakacode/rescript-react-on-rails
Please use those.

P.S. If they are out of date, please, update them.

---

client/app/bundles/comments/src/Actions/Actions.res line 20 at r2 (raw file):

  headers: {.}
}
@module("axios") external post: (string, storeCommentData, reqOptions) => promise<unit> = "post";

Is there a reason to use axios instead of fetch?

It seems like you don't use it.

---

client/app/bundles/comments/src/Actions/Actions.res line 53 at r2 (raw file):

    switch jsonRes->Json.decode(jsonComments) {
      | Ok(decodedRes) => decodedRes.comments
      | Error(err) => Js.Exn.raiseError(err)

In Rescript, use the result type instead of throwing an exception if an error is supposed to be handled by callers. Throw exceptions only in case of non-recoverable errors.

---

client/app/bundles/comments/src/Actions/Actions.res line 57 at r2 (raw file):

  }

  let storeComment = async (author: string, text: string) => {

It should be a record type.

---

client/app/bundles/comments/src/Actions/Actions.res line 63 at r2 (raw file):

        author: author,
        text: text
      },

It should be encoded.

---

client/app/bundles/comments/src/Actions/Actions.res line 89 at r2 (raw file):

    // } catch {
    //   | _ => setError(_ => true)
    // }

Remove.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 7 at r2 (raw file):

let make = (~storeComment: storeCommentAction, ~isSaving: bool) => {
  let (author, setAuthor) = React.useState(_ => "")
  let (text, setText) = React.useState(_ => "")

Define state as a record and use reducer to manage the state.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 8 at r2 (raw file):

  let (author, setAuthor) = React.useState(_ => "")
  let (text, setText) = React.useState(_ => "")
  let (currformType, setCurrFormType) = React.useState(_ => "Horizontal Form")

It should be a variant, not a string.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 22 at r2 (raw file):

  let handleSubmit = (e) => {
    ReactEvent.Form.preventDefault(e)
    storeComment(author, text)

What about error handling?

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 25 at r2 (raw file):

  }

  let horizontalForm = (

It should be separate React component rendered on demand only.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 51 at r2 (raw file):

  )

  let inlineForm = (

It should be separate React component rendered on demand only.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 67 at r2 (raw file):

  )

  let stackedForm = (

It should be separate React component rendered on demand only.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 91 at r2 (raw file):

  let handleClick = (formType: string): unit => {
    setCurrFormType(_ => formType)
    ();

Can be removed.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 100 at r2 (raw file):

      </li>
    )
  )

Avoid binding UI to variables like this. Either inline it in the UI tree or move it to a separate component.

---

client/app/bundles/comments/src/CommentList/CommentList.res line 12 at r2 (raw file):

  }

  let errorMsg = switch error {

Inline it.

---

client/app/bundles/comments/src/CommentList/CommentList.res line 14 at r2 (raw file):

  let errorMsg = switch error {
  | true => <AlertError cssTransitionGroupClassNames=cssTransitionGroupClassNames />
  | false => <></>

React.null

---

client/app/bundles/comments/src/CommentList/CommentList.res line 17 at r2 (raw file):

  }

  let commentsElements = Belt.Array.map(

Inline it.

Also, prefer piping: comments->Array.map(comment => ...).

---

client/app/bundles/comments/src/CommentList/AlertError/AlertError.res line 4 at r2 (raw file):

let make = (~cssTransitionGroupClassNames: CSSTransition.cssTransitionGroupClassNamesT) => {
  let nodeRef = React.useRef(Js.Nullable.null)
      

Configure your editor to autoformat files on save + remove trailing whitespaces if autoformatter is not available.

---

client/app/bundles/comments/src/CommentList/AlertError/AlertError.res line 6 at r2 (raw file):

      
  // The 500 must correspond to the 0.5s in:
  //   ../../RescriptPage.module.scss:9

You can export variables from SCSS to JS AFAIR.

---

client/app/bundles/comments/src/CommentList/AlertError/AlertError.res line 14 at r2 (raw file):

  >
    <div className="alert alert-danger">
      <strong>{React.string("Your comment was not saved!")}</strong>

Prefer piping.

---

client/app/bundles/comments/src/CommentList/Comment/Comment.res line 6 at r2 (raw file):

  gfm: bool
}
@module("marked") external marked: (string, markedOptions) => string = "marked";

Move it to own module: bindings/Marked.res

---

client/app/bundles/comments/src/CSSTransition/CSSTransition.res line 26 at r2 (raw file):

    ~classNames: cssTransitionGroupClassNamesT
  ) => React.element = "CSSTransition"
}

Move it to bindings/ dir.

---

client/app/packs/server-bundle.js line 11 at r2 (raw file):

import commentsStore from '../bundles/comments/store/commentsStore';
import Footer from '../bundles/comments/components/Footer/Footer';
import { make as RescriptPage } from '../bundles/comments/src/RescriptPage.bs.mjs';

In RescriptPage, you can do let default = make and then you can import RescriptPage from ....

---

spec/rescript/rescript_spec.rb line 78 at r2 (raw file):

    end
  end
end

Ask one of the Ruby devs to review this, please.

[Yassa-hue]

@ahangarha, Can you please review the rspec test that I added ?!

[ahangarha]

@ahangarha, Can you please review the rspec test that I added ?!

I will check it on Friday

[mightystrong]

client/app/bundles/comments/src/RescriptPage.res line 2 at r2 (raw file):

@react.component
let make = () => {

Change this to let default = () and remove the make from the ror registration. Only use make for components that are reusable and default for pages that are rendered by the rails view.

[mightystrong]

Reviewed 17 of 29 files at r1, 6 of 10 files at r2, all commit messages.
Reviewable status: all files reviewed, 54 unresolved discussions (waiting on @alex35mil and @Yassa-hue)

---

Procfile.dev line 7 at r2 (raw file):

rails: bundle exec rails s -p 3000
wp-client: HMR=true RAILS_ENV=development NODE_ENV=development bin/shakapacker-dev-server
wp-server: bundle exec rake react_on_rails:locale && HMR=true SERVER_BUNDLE_ONLY=yes bin/shakapacker --watch

Not sure you need HMR turned on for server but I defer to @justin808 on that one?

Here's what my Procfile.dev looks like after full installation of React on Rails / ReScript with creating an app from scratch with latest versions of Shakapacker & React on Rails, which works without all the ENV variables declared here. But I guess it doesn't hurt to have them.

rails: bundle exec rails s -p 3000
wp-client: bin/shakapacker-dev-server
wp-server: SERVER_BUNDLE_ONLY=yes bin/shakapacker --watch
rescript: yarn rescript build -with-deps -w

---

client/app/bundles/comments/src/RescriptPage.res line 2 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

Change this to let default = () and remove the make from the ror registration. Only use make for components that are reusable and default for pages that are rendered by the rails view.

Instead of calling this ReScriptPage maybe call it ReScriptShow.res which is in line with controller action def rescript which is basically similar to a show action for a model.

---

client/app/bundles/comments/src/RescriptPage.res line 5 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Refactor this: represent the state as a variant to eliminate the possibility of an invalid state and use reducer to manage the state.

See here for instructions -> https://rescript-lang.org/docs/react/latest/hooks-reducer

Example code from the book.

Code snippet:

module Container = ChapterContainer
module Page = ChapterPage

type action =
  | ToggleView(option<Preview.t>)
  | SetScrollPosition(float)

type state = {
  view: View.t,
  preview: option<Preview.t>,
  scrollPosition: float,
}

let reducer = (state, action) => {
  switch action {
    | ToggleView(preview) => {
      switch preview {
        | None => {...state, view: Chapter, preview: preview }
        | Some(RailsDefault)
        | Some(RescriptHelloWorld)
        | Some(HelloWorld)
        | Some(HelloWorldReact)
        | Some(RescriptHelloWorldTailwind)
        | Some(RescriptHelloWorldColor) =>
        {...state, view: Preview, preview: preview }
      }}

    | SetScrollPosition(position) => {...state, scrollPosition: position}
  }
}

@react.component
let default = (~chapter: Chapter.Pagination.t) => {
  let initialState = {
    view: Chapter,
    preview: None,
    scrollPosition: 0.0,
  }

  let (state, dispatch) = React.useReducer(reducer, initialState)

  let setScroll = (event, preview) => {
    event->Scroll.scrollTop->SetScrollPosition->dispatch
    ToggleView(Some(preview))->dispatch
  }

  let scrollTo = _ => {
    ToggleView(None)->dispatch
    state.scrollPosition->Scroll.AnimateScroll.scrollTo
  }

  let setScroll = (event, preview) => setScroll(event, preview)

  {switch state.view {
    | Chapter =>
      <FramerMotion.AnimatePresense>
        <Container previous={chapter.previous} next={chapter.next}>
          <Footnotes.Provider>
            <ChapterTypeIt messages=[`Chapter ${chapter.current.number->Js.Int.toString}`, chapter.current.title] />
            <Page>
              {switch chapter.current.number {
                | 1 => <Chapter01 setScroll />
                | 2 => <Chapter02 setScroll />
                | 3 => <Chapter03 setScroll />
                | _ => React.null
              }}
              <Footnotes />
            </Page>
          </Footnotes.Provider>
        </Container>
      </FramerMotion.AnimatePresense>
    | Preview =>
      <FramerMotion.AnimatePresense>
        <PreviewBrowser onClick={scrollTo}>
          {switch state.preview {
            | Some(RailsDefault) => <PreviewRailsDefault />
            | Some(HelloWorld) => <PreviewHelloWorld />
            | Some(HelloWorldReact) => <PreviewHelloWorldReact />
            | Some(RescriptHelloWorld) => <PreviewRescriptHelloWorld color={false} />
            | Some(RescriptHelloWorldTailwind) => <PreviewRescriptHelloWorldTailwind />
            | Some(RescriptHelloWorldColor) => <PreviewRescriptHelloWorld color={true} />
            | None => React.null
          }}
        </PreviewBrowser>
      </FramerMotion.AnimatePresense>
  }}
}

---

client/app/bundles/comments/src/RescriptPage.bs.mjs line 1 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

We usually gitignore this.

Yes. Don’t commit the rescript generated files. These get generated when assets are precompiled for production.

---

client/app/bundles/comments/src/vars.scss line 10 at r2 (raw file):

// It will be used in SASS components imported as CSS Modules
$comment-author-color: blue;
$comment-text-color: purple;

new line

---

client/app/bundles/comments/src/Actions/Actions.res line 20 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Is there a reason to use axios instead of fetch?

It seems like you don't use it.

This is subjective, but as an organizational convention, I prefer to put bindings in a separate location and then name the primary binding file after the javascript. In this case, Axios.res. This way, there's a place for bindings as the library of binding functions grows. It makes the code easier to maintain as well as packages upgrade.

And, since this a Rails application, it makes sense to organize in a Rails-like way. So, for comments you would have a Comment.res model file with a type t that parallels the Rails model and its fields. In this case you'd have this instead of defining type storeCommentData =...

Code snippet:

// Comment.res
// First, definte a model the exactly parallels the Rails model and fields.
type t = {
  author: string,
  text: string,
  createdAt: ..,
  updatedAt: ..,
}

// The define modules with special purposes like a form.
module Create = {
 type t = {
   author: string,
   text: string, 
}

// Another special purpose
module Tile = {
  type t = ...
}

// Then Comment.t and Comment.Create.t can be used throughout the ReScript, including defining API calls.
// Other developers can easily find the matching ReScript model by using the name as the Rails model.
// app/models/comment.rb & app/javascript/models/Comment.res
// app/controllers/api/comments_controller.rb & app/javascript/api/comments/Api_Comment.res

---

client/app/bundles/comments/src/Actions/Actions.bs.mjs line 1 at r2 (raw file):

// Generated by ReScript, PLEASE EDIT WITH CARE

As per above don’t commit

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 8 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

It should be a variant, not a string.

type form = 
    | Horizontal
    | Vertical
    | Whatever

let (currformType, setCurrFormType) = React.useState(_ => Horizontal)

switch state.formType {
    | Horizontal => <HorizontalForm /> 
    | Vertical => <VeritcalForm />
    | ...until ur done
}

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 10 at r2 (raw file):

  let (currformType, setCurrFormType) = React.useState(_ => "Horizontal Form")

  let handleAuthorChange = (e) => {

All of these handle change calls would be unnecessary if using a reducer.

onClick={ event => SetFormType(Horizontal)->dispatch }

onChange={event => 
    ReactEvent.Form.preventDefault(event)
    let value = ReactEvent.Form.target(event)["value"]
    SetAuthor(value)->dispatch
}

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 25 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

It should be separate React component rendered on demand only.

Agreed.

// 
@react.component
let make

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 103 at r2 (raw file):

  let form = switch currformType {
  | "Horizontal Form" => horizontalForm

See above. Should be a Variant -> https://rescript-lang.org/docs/manual/latest/variant#sidebar

String combinations are nearly infinite. Variants are purposely limited. So below you convert to:

| Horizontal => do something
| Stacked => do something else
| Inline => another thing
| Other
| Cool => these variants both do this
| _ => only use this if your have more variants, and they all do the same thing.

If for some reason you need to convert a variant to a string. No prob.

let formTypeToSting = formType =>
switch formType {
| Horizontal => "Horizontal Form"
| ....etc.

formType->formTypeToString

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 117 at r2 (raw file):

  </div>

}

new line

---

client/app/bundles/comments/src/CommentForm/CommentForm.bs.mjs line 1 at r2 (raw file):

// Generated by ReScript, PLEASE EDIT WITH CARE

Remove from commit.

---

client/app/bundles/comments/src/CommentList/CommentList.res line 12 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Inline it.

error ? <Alert /> : React.null

---

client/app/bundles/comments/src/CommentList/CommentList.res line 17 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Inline it.

Also, prefer piping: comments->Array.map(comment => ...).

Agreed. In most cases, piping is more readable and is similar to "." notation. some.javascript.function() some->javascript->function()

---

client/app/bundles/comments/src/CommentList/CommentList.bs.mjs line 1 at r2 (raw file):

// Generated by ReScript, PLEASE EDIT WITH CARE

Remove from commit.

---

client/app/bundles/comments/src/CommentList/AlertError/AlertError.res line 2 at r2 (raw file):

@react.component
let make = (~cssTransitionGroupClassNames: CSSTransition.cssTransitionGroupClassNamesT) => {

CSSTransition.cssTransitionGroupClassNamesT is a super long type definition and it repeats the parent module name.` I haven't reviewed this file, but this might make it more readable.

// CSSTransition.res

.
.
.
module GroupClassNames = {
    type t = {
     ...
    }
}

CSSTransition.GroupClassNames.t

---

client/app/bundles/comments/src/CommentList/AlertError/AlertError.res line 17 at r2 (raw file):

    </div>
  </CSSTransition.CSSTransition>
}

new line

---

client/app/bundles/comments/src/CommentList/AlertError/AlertError.bs.mjs line 1 at r2 (raw file):

// Generated by ReScript, PLEASE EDIT WITH CARE

Remove

---

client/app/bundles/comments/src/CommentList/Comment/Comment.res line 6 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Move it to own module: bindings/Marked.res

See comments above about organizing bindings.

---

client/app/bundles/comments/src/CommentList/Comment/Comment.res line 28 at r2 (raw file):

  </CSSTransition.CSSTransition>
  
}

new line

---

client/app/bundles/comments/src/CommentList/Comment/Comment.bs.mjs line 1 at r2 (raw file):

// Generated by ReScript, PLEASE EDIT WITH CARE

remove

---

client/app/bundles/comments/src/CSSTransition/CSSTransition.res line 10 at r2 (raw file):

}

type cssTransitionGroupClassNamesT = {

as commented above, I'd make this it's own module GroupClassNames with type T.

---

client/app/bundles/comments/src/CSSTransition/CSSTransition.res line 17 at r2 (raw file):

}

module CSSTransition = {

I typically avoid have a module inside of a file with the same name as the file. I think you can remove the module declaration all-together here. So, you can just call <CSSTransition ~props /> instead of <CSSTransition.CSSTransition ~props />

---

client/app/bundles/comments/src/CSSTransition/CSSTransition.res line 24 at r2 (raw file):

    ~timeout: int, 
    ~nodeRef: React.ref<Js.Nullable.t<'a>>, 
    ~classNames: cssTransitionGroupClassNamesT

This just becomes GroupClassNames.t since it's inferred by ReScript form the parent module.

---

client/app/bundles/comments/src/CSSTransition/CSSTransition.bs.mjs line 1 at r2 (raw file):

// Generated by ReScript, PLEASE EDIT WITH CARE

remove

---

client/app/bundles/comments/src/Header/Header.res line 7 at r2 (raw file):

      <li>
        <a href="https://blog.shakacode.com/can-shakacode-help-you-4a5b1e5a8a63#.jex6tg9w9">
          {"Can ShakaCode Help You? "->React.string}

You use piping here, but parentheses below {React.string("shakacode/react-native-tutorial")}. I'd stick with piping as a general rule. For example, with long strings that have embedded values it's more readable.

{`This is a really \
    really, really, really \
    long string with ${data}`->React.string}

---

client/app/bundles/comments/src/Header/Header.res line 51 at r2 (raw file):

    <hr/>
  </>
}

new line

---

client/app/bundles/comments/src/Header/Header.bs.mjs line 1 at r2 (raw file):

// Generated by ReScript, PLEASE EDIT WITH CARE

remove

---

client/app/packs/client-bundle.js line 14 at r2 (raw file):

import NavigationBarApp from '../bundles/comments/startup/NavigationBarApp';
import Footer from '../bundles/comments/components/Footer/Footer';
import { make as RescriptPage } from '../bundles/comments/src/RescriptPage.bs.mjs';

Change let make = to let default = in the component and remove the make as

[mightystrong]

Reviewed 7 of 29 files at r1, 4 of 10 files at r2.
Reviewable status: all files reviewed, 54 unresolved discussions (waiting on @alex35mil and @Yassa-hue)

[mightystrong]

client/app/bundles/comments/src/CommentForm/CommentForm.res line 25 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

Agreed.

// 
@react.component
let make

// HorizontalForm.res
@react.component
let make = () => 
. . .

[Yassa-hue]

Reviewable status: all files reviewed, 54 unresolved discussions (waiting on @alex35mil, @justin808, and @mightystrong)

---

.gitignore line 51 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Ignore the whole lib as there might be other directories in there, such as ocaml.

The lib has other important dirs like the tasks dir!

---

bsconfig.json line 15 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Add -open Belt.

Done.

---

bsconfig.json line 16 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

You can remove this one.

Done.

---

bsconfig.json line 27 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Format.

What do you mean by format?!

---

bsconfig.json line 30 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Configure your editor to add new lines at the end of files on save.

Done.

---

Procfile.dev line 7 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

Not sure you need HMR turned on for server but I defer to @justin808 on that one?

Here's what my Procfile.dev looks like after full installation of React on Rails / ReScript with creating an app from scratch with latest versions of Shakapacker & React on Rails, which works without all the ENV variables declared here. But I guess it doesn't hurt to have them.

rails: bundle exec rails s -p 3000
wp-client: bin/shakapacker-dev-server
wp-server: SERVER_BUNDLE_ONLY=yes bin/shakapacker --watch
rescript: yarn rescript build -with-deps -w

I don't know if I should remove the ENV variables as It is not that relevant to my PR. What do you think?!

---

client/app/bundles/comments/src/RescriptPage.res line 2 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

Instead of calling this ReScriptPage maybe call it ReScriptShow.res which is in line with controller action def rescript which is basically similar to a show action for a model.

Done.

---

client/app/bundles/comments/src/RescriptPage.res line 5 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

See here for instructions -> https://rescript-lang.org/docs/react/latest/hooks-reducer

Example code from the book.

Done.

---

client/app/bundles/comments/src/RescriptPage.res line 9 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

You don't need let _ = ... if the return value is unit.

Done.

---

client/app/bundles/comments/src/RescriptPage.bs.mjs line 1 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

We usually gitignore this.

At the rescript docs I found that it's best practice to leave it so that we can see how the changes on the .res files affect the outputted .mjs files.

What do you think?!

---

client/app/bundles/comments/src/vars.scss line 10 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

new line

Done.

---

client/app/bundles/comments/src/Actions/Actions.res line 8 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

We have these bindings: https://github.com/shakacode/rescript-react-on-rails
Please use those.

P.S. If they are out of date, please, update them.

It didn't work for me. Are there any missing installation steps?!

---

client/app/bundles/comments/src/Actions/Actions.res line 20 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

This is subjective, but as an organizational convention, I prefer to put bindings in a separate location and then name the primary binding file after the javascript. In this case, Axios.res. This way, there's a place for bindings as the library of binding functions grows. It makes the code easier to maintain as well as packages upgrade.

And, since this a Rails application, it makes sense to organize in a Rails-like way. So, for comments you would have a Comment.res model file with a type t that parallels the Rails model and its fields. In this case you'd have this instead of defining type storeCommentData =...

axios is just easier to configure with the ROR headers.

---

client/app/bundles/comments/src/Actions/Actions.res line 53 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

In Rescript, use the result type instead of throwing an exception if an error is supposed to be handled by callers. Throw exceptions only in case of non-recoverable errors.

Done

---

client/app/bundles/comments/src/Actions/Actions.res line 57 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

It should be a record type.

Done.

---

client/app/bundles/comments/src/Actions/Actions.res line 63 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

It should be encoded.

It works as it is, I used the js example at
https://github.com/shakacode/react-webpack-rails-tutorial/blob/master/client/app/bundles/comments/components/SimpleCommentScreen/SimpleCommentScreen.jsx

---

client/app/bundles/comments/src/Actions/Actions.res line 89 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Remove.

Done.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 7 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Define state as a record and use reducer to manage the state.

Done.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 8 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

It should be a variant, not a string.

Done.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 10 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

All of these handle change calls would be unnecessary if using a reducer.

onClick={ event => SetFormType(Horizontal)->dispatch }

onChange={event => 
    ReactEvent.Form.preventDefault(event)
    let value = ReactEvent.Form.target(event)["value"]
    SetAuthor(value)->dispatch
}

But we'll duplicate this logic multiple times on the 3 forms. Wouldn't be more organized if we put this logic on a function (handleAuthorChange) and path it to the forms?

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 22 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

What about error handling?

Error handling is in the ../RescriptPage.res:7

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 25 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

It should be separate React component rendered on demand only.

Done.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 51 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

It should be separate React component rendered on demand only.

Done.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 67 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

It should be separate React component rendered on demand only.

Done.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 91 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Can be removed.

Done.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 100 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Avoid binding UI to variables like this. Either inline it in the UI tree or move it to a separate component.

Done.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 103 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

See above. Should be a Variant -> https://rescript-lang.org/docs/manual/latest/variant#sidebar

String combinations are nearly infinite. Variants are purposely limited. So below you convert to:

| Horizontal => do something
| Stacked => do something else
| Inline => another thing
| Other
| Cool => these variants both do this
| _ => only use this if your have more variants, and they all do the same thing.

If for some reason you need to convert a variant to a string. No prob.

let formTypeToSting = formType =>
switch formType {
| Horizontal => "Horizontal Form"
| ....etc.

formType->formTypeToString

Done.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 117 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

new line

Done.

---

client/app/bundles/comments/src/CommentList/CommentList.res line 12 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Inline it.

Done.

---

client/app/bundles/comments/src/CommentList/CommentList.res line 14 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

React.null

Done.

---

client/app/bundles/comments/src/CommentList/CommentList.res line 17 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Inline it.

Also, prefer piping: comments->Array.map(comment => ...).

Done.

---

client/app/bundles/comments/src/CommentList/AlertError/AlertError.res line 2 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

CSSTransition.cssTransitionGroupClassNamesT is a super long type definition and it repeats the parent module name.` I haven't reviewed this file, but this might make it more readable.

// CSSTransition.res

.
.
.
module GroupClassNames = {
    type t = {
     ...
    }
}

CSSTransition.GroupClassNames.t

Done.

---

client/app/bundles/comments/src/CommentList/AlertError/AlertError.res line 6 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

You can export variables from SCSS to JS AFAIR.

Can you explain more?!

---

client/app/bundles/comments/src/CommentList/AlertError/AlertError.res line 14 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Prefer piping.

Done.

---

client/app/bundles/comments/src/CommentList/AlertError/AlertError.res line 17 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

new line

Done.

---

client/app/bundles/comments/src/CommentList/Comment/Comment.res line 6 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Move it to own module: bindings/Marked.res

Done.

---

client/app/bundles/comments/src/CommentList/Comment/Comment.res line 28 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

new line

Done.

---

client/app/bundles/comments/src/CSSTransition/CSSTransition.res line 10 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

as commented above, I'd make this it's own module GroupClassNames with type T.

Done.

---

client/app/bundles/comments/src/CSSTransition/CSSTransition.res line 17 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

I typically avoid have a module inside of a file with the same name as the file. I think you can remove the module declaration all-together here. So, you can just call <CSSTransition ~props /> instead of <CSSTransition.CSSTransition ~props />

How about renaming?!

---

client/app/bundles/comments/src/CSSTransition/CSSTransition.res line 24 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

This just becomes GroupClassNames.t since it's inferred by ReScript form the parent module.

Done.

---

client/app/bundles/comments/src/CSSTransition/CSSTransition.res line 26 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Move it to bindings/ dir.

Done.

---

client/app/bundles/comments/src/Header/Header.res line 7 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

You use piping here, but parentheses below {React.string("shakacode/react-native-tutorial")}. I'd stick with piping as a general rule. For example, with long strings that have embedded values it's more readable.

{`This is a really \
    really, really, really \
    long string with ${data}`->React.string}

I'm not sure I understand you here.

---

client/app/bundles/comments/src/Header/Header.res line 51 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

new line

Done.

---

client/app/packs/client-bundle.js line 14 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

Change let make = to let default = in the component and remove the make as

Done.

---

client/app/packs/server-bundle.js line 11 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

In RescriptPage, you can do let default = make and then you can import RescriptPage from ....

Done.

---

spec/rescript/rescript_spec.rb line 78 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Ask one of the Ruby devs to review this, please.

Done.

[Yassa-hue]

@ahangarha , I made the rspec tests wait for 10s before clicking the links and the test passed locally. What do you think?!

[mightystrong]

Reviewed 33 of 34 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 57 unresolved discussions (waiting on @alex35mil, @justin808, and @Yassa-hue)

---

client/app/bundles/comments/src/RescriptPage.res line 2 at r2 (raw file):

Previously, Yassa-hue (Yassa Tamer) wrote…

Done.

@Yassa-hue This still is a let make function and should be let default so you don't have to call make in the registration files. In our context make is essentially reserved for child components. Default is used for components rendered by the rails view.

---

client/app/bundles/comments/src/RescriptPage.res line 1 at r4 (raw file):

type rescriptPageStateT = {

I personally wouldn't name these like this since you're repeating the name of the .res file. Instead, I would name type state = {. So this is more readable throughout the rest of the file.

---

client/app/bundles/comments/src/RescriptPage.res line 7 at r4 (raw file):

}

type rescriptPageActionT =

type action = {

---

client/app/bundles/comments/src/RescriptPage.res line 14 at r4 (raw file):

let reducer = (
  state: rescriptPageStateT, 

This can just be let reducer = (state, action) since state and action are inferred by the types defined above within the module.

---

client/app/bundles/comments/src/RescriptPage.res line 84 at r4 (raw file):

}

let default = make

This is not needed if simply defining has let default at the beginning. Unless the component will be used later as the child of another component.

---

client/app/bundles/comments/src/RescriptPage.bs.mjs line 1 at r2 (raw file):

Previously, Yassa-hue (Yassa Tamer) wrote…

At the rescript docs I found that it's best practice to leave it so that we can see how the changes on the .res files affect the outputted .mjs files.

What do you think?!

If the ReScript works, in 99.9% of the cases, the compiled JavaScript file will work. These will also be automatically generated when spinning up the server or when HMR fires. So, the only reason I can see to keep these committed is for educational purposes only. But in a production app, we wouldn't commit since they're be generated when you run rails assets:precompile

---

client/app/bundles/comments/src/Actions/Actions.res line 35 at r4 (raw file):

  }

  let fetchComments = async (): result<comments, Types.errorT> => {

Remove the "T" in your type definitions.

---

client/app/bundles/comments/src/Actions/Actions.bs.mjs line 1 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

As per above don’t commit

Don't commit.

---

client/app/bundles/comments/src/Actions/Types.res line 1 at r4 (raw file):

type formDisplayT = HorizontalForm | InlineForm | StackedFrom;

In my option, the capital Ts at the end of type definitions is unnecessary. I would also name the file specific to its purpose. In a larger application, Types could mean anything. This appears to be related to a form used by a model. So, I'd refactor this in the following way. Semi-colons are not needed.

type display = Horizontal | Inline | Stacked // Form is repetitive and inferred by a more descriptive file name of CommentForm.res

type state = {
  author: string,
  text: string,
  form: display,
}

type action = {
  | SetAuthor(string)

.
.
.

---

client/app/bundles/comments/src/Actions/Types.bs.mjs line 1 at r4 (raw file):

// Generated by ReScript, PLEASE EDIT WITH CARE

Remove

---

client/app/bundles/comments/src/bindings/Axios.bs.mjs line 1 at r4 (raw file):

// Generated by ReScript, PLEASE EDIT WITH CARE

remove

---

client/app/bundles/comments/src/bindings/Marked.bs.mjs line 1 at r4 (raw file):

// Generated by ReScript, PLEASE EDIT WITH CARE

Remove

---

client/app/bundles/comments/src/bindings/ReactOnRails.res line 2 at r4 (raw file):

type rorDefault = {
  authenticityHeaders: unit => {.} 

Remove all semi-colons.

---

client/app/bundles/comments/src/bindings/ReactOnRails.bs.mjs line 1 at r4 (raw file):

// Generated by ReScript, PLEASE EDIT WITH CARE

Remove

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 4 at r4 (raw file):

let reducer = (
  state: Types.commentFormStateT, 

Remove all the trailing "T"s from type definitions.

---

client/app/bundles/comments/src/CommentForm/CommentForm.bs.mjs line 1 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

Remove from commit.

Remove

---

client/app/bundles/comments/src/CommentForm/forms/HorizontalForm.bs.mjs line 1 at r4 (raw file):

// Generated by ReScript, PLEASE EDIT WITH CARE

remove from commit

---

client/app/bundles/comments/src/CommentForm/forms/InlineForm.bs.mjs line 1 at r4 (raw file):

// Generated by ReScript, PLEASE EDIT WITH CARE

remove

---

client/app/bundles/comments/src/CommentForm/forms/StackedFrom.bs.mjs line 1 at r4 (raw file):

// Generated by ReScript, PLEASE EDIT WITH CARE

remove

---

client/app/packs/client-bundle.js line 14 at r2 (raw file):

Previously, Yassa-hue (Yassa Tamer) wrote…

Done.

@Yassa-hue In order for this to work let make has to be changed to let default in the RescriptPage.res file.

---

client/app/bundles/comments/src/CSSTransition/CSSTransition.bs.mjs line 1 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

remove

Remove

[Yassa-hue]

Reviewable status: 14 of 48 files reviewed, 57 unresolved discussions (waiting on @alex35mil, @justin808, and @mightystrong)

---

client/app/bundles/comments/src/Actions/Actions.res line 35 at r4 (raw file):

Previously, mightystrong (Michael Price) wrote…

Remove the "T" in your type definitions.

Done.

---

client/app/bundles/comments/src/Actions/Types.res line 1 at r4 (raw file):

Previously, mightystrong (Michael Price) wrote…

In my option, the capital Ts at the end of type definitions is unnecessary. I would also name the file specific to its purpose. In a larger application, Types could mean anything. This appears to be related to a form used by a model. So, I'd refactor this in the following way. Semi-colons are not needed.

type display = Horizontal | Inline | Stacked // Form is repetitive and inferred by a more descriptive file name of CommentForm.res

type state = {
  author: string,
  text: string,
  form: display,
}

type action = {
  | SetAuthor(string)

.
.
.

Done.

I'll keep the type name formDisplay without the T as display only is not entirely descriptive for me.

Also, I found similar files with the same name at other projects, so I thought applying in this PR was a good idea.

---

client/app/bundles/comments/src/bindings/ReactOnRails.res line 2 at r4 (raw file):

Previously, mightystrong (Michael Price) wrote…

Remove all semi-colons.

Done.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 4 at r4 (raw file):

Previously, mightystrong (Michael Price) wrote…

Remove all the trailing "T"s from type definitions.

Done.

---

client/app/packs/client-bundle.js line 14 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

@Yassa-hue In order for this to work let make has to be changed to let default in the RescriptPage.res file.

Done.

---

client/app/bundles/comments/src/RescriptPage.bs.mjs line 1 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

If the ReScript works, in 99.9% of the cases, the compiled JavaScript file will work. These will also be automatically generated when spinning up the server or when HMR fires. So, the only reason I can see to keep these committed is for educational purposes only. But in a production app, we wouldn't commit since they're be generated when you run rails assets:precompile

I agree, I'll remove them.

---

client/app/bundles/comments/src/RescriptPage.res line 2 at r2 (raw file):

Previously, mightystrong (Michael Price) wrote…

@Yassa-hue This still is a let make function and should be let default so you don't have to call make in the registration files. In our context make is essentially reserved for child components. Default is used for components rendered by the rails view.

Done.

I was just confused between your suggestion and @alex35mil 's. Any way I applied yours.

---

client/app/bundles/comments/src/RescriptPage.res line 1 at r4 (raw file):

Previously, mightystrong (Michael Price) wrote…

I personally wouldn't name these like this since you're repeating the name of the .res file. Instead, I would name type state = {. So this is more readable throughout the rest of the file.

Done.

---

client/app/bundles/comments/src/RescriptPage.res line 7 at r4 (raw file):

Previously, mightystrong (Michael Price) wrote…

type action = {

Done.

---

client/app/bundles/comments/src/RescriptPage.res line 14 at r4 (raw file):

Previously, mightystrong (Michael Price) wrote…

This can just be let reducer = (state, action) since state and action are inferred by the types defined above within the module.

Done.

---

client/app/bundles/comments/src/RescriptPage.res line 84 at r4 (raw file):

Previously, mightystrong (Michael Price) wrote…

This is not needed if simply defining has let default at the beginning. Unless the component will be used later as the child of another component.

Done.

[alex35mil]

I left comments.

Also, it's a weird place to put this example (client/app/bundles/comments/src ). At least, rename src to rescript.

Reviewed 1 of 34 files at r3, 2 of 3 files at r4, 21 of 34 files at r5, 1 of 3 files at r6, 14 of 18 files at r7, all commit messages.
Reviewable status: 41 of 51 files reviewed, 54 unresolved discussions (waiting on @justin808, @mightystrong, and @Yassa-hue)

---

.gitignore line 51 at r2 (raw file):

Previously, Yassa-hue (Yassa Tamer) wrote…

The lib has other important dirs like the tasks dir!

Then ignore lib/ocaml.

---

bsconfig.json line 27 at r2 (raw file):

Previously, Yassa-hue (Yassa Tamer) wrote…

What do you mean by format?!

I mean this line should have indentation.

---

bsconfig.json line 16 at r7 (raw file):

  ],
  "bsc-flags": ["-open JsonCombinators", "-open Belt"],
  "suffix": ".bs.mjs",

Replace mjs with js. This should solve your issue with rescript-react-on-rails.

---

package.json line 93 at r4 (raw file):

    "redux-thunk": "^2.2.0",
    "rescript": "^10.1.4",
    "rescript-react-on-rails": "^1.0.0",

Get this back please.

---

app/views/pages/rescript.html.erb line 1 at r6 (raw file):

<%= react_component "RescriptShow", prerender: false %>

Why prerender: false?

---

client/app/bundles/comments/src/Actions/Actions.res line 8 at r2 (raw file):

Previously, Yassa-hue (Yassa Tamer) wrote…

It didn't work for me. Are there any missing installation steps?!

See my comment above.

---

client/app/bundles/comments/src/Actions/Actions.res line 31 at r7 (raw file):

  type comments = array<t>

  type commentsResT = {

Remove T.

---

client/app/bundles/comments/src/Actions/Actions.res line 38 at r7 (raw file):

    open Json.Decode

    let response = await Fetch.get("comments.json")

I don't get it. You use both fetch and axios?

---

client/app/bundles/comments/src/Actions/Types.res line 28 at r7 (raw file):

type error = NoError | FailedToSaveComment | FailedToFetchComments

type isSaving = Free | BusySaving

You should remove this module entirely and define local types for each interaction. Data fetch error is not related to data saving error and should not appear in the same type. More over, error, as well as data (if any), should be a part of the status type as these entities are available only in certain contexts.

See https://fsharpforfunandprofit.com/posts/designing-with-types-making-illegal-states-unrepresentable/ (different language, but the concept is exactly the same).

---

client/app/bundles/comments/src/bindings/ReactOnRails.res line 7 at r7 (raw file):

  default: rorDefault
}
@module("react-on-rails") external ror: rorModule = "default"

Replace this module with rescript-react-on-rails.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 22 at r2 (raw file):

Previously, Yassa-hue (Yassa Tamer) wrote…

Error handling is in the ../RescriptPage.res:7

It should be redone. See my previous comments.

---

client/app/bundles/comments/src/CommentForm/forms/HorizontalForm.res line 10 at r7 (raw file):

  ~isSaving: Types.isSaving
  ) => {
  <form className="form-horizontal flex flex-col gap-4"  onSubmit=handleSubmit disabled={isSaving == BusySaving}>

Use pattern matching instead of ==. It gives exhaustiveness checking to make sure all cases are handled when the type is extended.

---

client/app/bundles/comments/src/CommentForm/forms/InlineForm.res line 13 at r7 (raw file):

    className="form-inline flex flex-col lg:flex-row flex-wrap gap-4"
    onSubmit=handleSubmit
    disabled={isSaving == BusySaving}

Same.

---

client/app/bundles/comments/src/CommentForm/forms/StackedFrom.res line 12 at r7 (raw file):

  <form 
    onSubmit=handleSubmit 
    disabled={isSaving == BusySaving}

Same.

---

client/app/bundles/comments/src/CommentList/CommentList.res line 18 at r7 (raw file):

    <CSSAnimation.TransitionGroup className="commentList" component="div">
      {
        comments->Belt.Array.map(

FYI You don't need Belt prefix since it is globally opened via bsconfig.json.

---

client/app/bundles/comments/src/CommentList/AlertError/AlertError.res line 6 at r2 (raw file):

Previously, Yassa-hue (Yassa Tamer) wrote…

Can you explain more?!

There are different options:
https://github.com/webpack-contrib/css-loader#separating-interoperable-css-only-and-css-module-features
https://github.com/Coobaha/postcss-variables-loader

---

client/app/bundles/comments/src/ReScriptShow.res line 3 at r7 (raw file):

type state = {
  comments: Actions.Fetch.comments,
  error: Types.error,

Error should be a part of the status type.

---

client/app/bundles/comments/src/ReScriptShow.res line 4 at r7 (raw file):

  comments: Actions.Fetch.comments,
  error: Types.error,
  isSaving: Types.isSaving

This is confusing. Why Types module? It should be a local type called something like status, not isSaving.

---

client/app/bundles/comments/src/ReScriptShow.res line 27 at r7 (raw file):

let default = () => {
  let (state, dispatch) = React.useReducer(
    reducer, {

Setup rescript integration in your editor. It will enable auto-formatting.

---

client/app/bundles/comments/src/CSSTransition/CSSTransition.res line 26 at r2 (raw file):

Previously, Yassa-hue (Yassa Tamer) wrote…

Done.

And rename the module to ReactTransitionGroup.

[justin808]

@Yassa-hue let me know when ready to merge. @Judahmeek can do it and push it...

[Yassa-hue]

@justin808, the main functions are done (fetch the comments and add a new comment), I also used our React on Rails binding instead of my binding (that was the main issue that blocked the PR). What is left is some improvements on the design I think I can complete them today. (cc @alex35mil)

[Yassa-hue]

Done!

Reviewable status: 17 of 56 files reviewed, 54 unresolved discussions (waiting on @alex35mil, @justin808, and @mightystrong)

---

.gitignore line 51 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Then ignore lib/ocaml.

Done.

---

bsconfig.json line 27 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

I mean this line should have indentation.

Sorry! I can't get the problem yet :")

---

bsconfig.json line 16 at r7 (raw file):

Previously, alex35mil (Alex) wrote…

Replace mjs with js. This should solve your issue with rescript-react-on-rails.

Done.

---

package.json line 93 at r4 (raw file):

Previously, alex35mil (Alex) wrote…

Get this back please.

Done.

---

app/views/pages/rescript.html.erb line 1 at r6 (raw file):

Previously, alex35mil (Alex) wrote…

Why prerender: false?

I thought the main goal of this PR was to make a rescript version of the simple react page which is prerender: false but it seems that it should be pre-rendered. I'll set it to true.

---

client/app/bundles/comments/src/bindings/ReactOnRails.res line 7 at r7 (raw file):

Previously, alex35mil (Alex) wrote…

Replace this module with rescript-react-on-rails.

Done.

---

client/app/bundles/comments/src/CommentList/AlertError/AlertError.res line 4 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

Configure your editor to autoformat files on save + remove trailing whitespaces if autoformatter is not available.

I used the compiler formatter to format all the files at once and also added the res:format script to the package.json file

---

client/app/bundles/comments/src/CommentList/AlertError/AlertError.res line 6 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

There are different options:
https://github.com/webpack-contrib/css-loader#separating-interoperable-css-only-and-css-module-features
https://github.com/Coobaha/postcss-variables-loader

We don't use SCSS vars anymore. I'm not sure what you mean.

---

client/app/bundles/comments/src/ReScriptShow.res line 3 at r7 (raw file):

Previously, alex35mil (Alex) wrote…

Error should be a part of the status type.

Done.

---

client/app/bundles/comments/src/ReScriptShow.res line 4 at r7 (raw file):

Why Types module?

Because this type is needed in multiple files. When I tried to put it on the RescriptShow.res and call it in CommentForm.res I faced a dependency cycle error. My idea is to use a file that contains all types that are used in several files in the Types.res file. I agree the naming is not the best we can do. I suggest making a types file for the ReScriptShow.res component and putting on it all the types we need. We can call it a local type then.

Regarding this line of code specifically:
I don't think the form needs to know the status of the saving anyway it just needs a bool value to indicate if it should be disabled or not so I'll create the local type as you suggested and pass only a bool value disabled to the form.

---

client/app/bundles/comments/src/ReScriptShow.res line 27 at r7 (raw file):

Previously, alex35mil (Alex) wrote…

Setup rescript integration in your editor. It will enable auto-formatting.

Done! I used the compiler format.

---

client/app/bundles/comments/src/Actions/Actions.res line 8 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

See my comment above.

Done! They aren't out of date the js file extension was wrong (as you explained on Slack). I found that we don't need to bind the authenticityHeaders we can use the authenticityToken function to set the headers directly. see the implementation.

---

client/app/bundles/comments/src/Actions/Actions.res line 31 at r7 (raw file):

Previously, alex35mil (Alex) wrote…

Remove T.

Done.

---

client/app/bundles/comments/src/Actions/Actions.res line 38 at r7 (raw file):

Previously, alex35mil (Alex) wrote…

I don't get it. You use both fetch and axios?

As I said above, this page is meant to be a rescript copy of the simple React page. we use Axios because we set the headers during the post request and Fetch because it's a simple and direct fetch. It's the same as we do in the simple React page.

---

client/app/bundles/comments/src/Actions/Types.res line 28 at r7 (raw file):

Previously, alex35mil (Alex) wrote…

You should remove this module entirely and define local types for each interaction. Data fetch error is not related to data saving error and should not appear in the same type. More over, error, as well as data (if any), should be a part of the status type as these entities are available only in certain contexts.

See https://fsharpforfunandprofit.com/posts/designing-with-types-making-illegal-states-unrepresentable/ (different language, but the concept is exactly the same).

I've separated the errors and put them in the status object with the saving status.

---

client/app/bundles/comments/src/CommentForm/CommentForm.res line 22 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

It should be redone. See my previous comments.

I've redesigned the errors on the page completely.

---

client/app/bundles/comments/src/CommentForm/forms/HorizontalForm.res line 10 at r7 (raw file):

Previously, alex35mil (Alex) wrote…

Use pattern matching instead of ==. It gives exhaustiveness checking to make sure all cases are handled when the type is extended.

Done.

---

client/app/bundles/comments/src/CommentForm/forms/InlineForm.res line 13 at r7 (raw file):

Previously, alex35mil (Alex) wrote…

Same.

Done.

---

client/app/bundles/comments/src/CommentForm/forms/StackedFrom.res line 12 at r7 (raw file):

Previously, alex35mil (Alex) wrote…

Same.

Done.

---

client/app/bundles/comments/src/CommentList/CommentList.res line 18 at r7 (raw file):

Previously, alex35mil (Alex) wrote…

FYI You don't need Belt prefix since it is globally opened via bsconfig.json.

Done

---

client/app/bundles/comments/src/CSSTransition/CSSTransition.res line 26 at r2 (raw file):

Previously, alex35mil (Alex) wrote…

And rename the module to ReactTransitionGroup.

Done.

[alex35mil]

Reviewed 4 of 45 files at r5, 3 of 25 files at r7, 7 of 38 files at r8, 15 of 19 files at r9, 20 of 20 files at r10, all commit messages.
Reviewable status: 66 of 69 files reviewed, 28 unresolved discussions (waiting on @justin808, @mightystrong, and @Yassa-hue)

---

client/app/bundles/comments/rescript/bindings/Axios.res line 9 at r10 (raw file):

  author: string,
  text: string,
}

It is unrelated to axios, so it should be moved elsewhere.

---

client/app/bundles/comments/rescript/Header/Header.bs.mjs line 95 at r10 (raw file):

  make ,
}
/* react/jsx-runtime Not a pure module */

Remove.

---

client/app/bundles/comments/src/Actions/Actions.res line 38 at r7 (raw file):

Previously, Yassa-hue (Yassa Tamer) wrote…

As I said above, this page is meant to be a rescript copy of the simple React page. we use Axios because we set the headers during the post request and Fetch because it's a simple and direct fetch. It's the same as we do in the simple React page.

Regardless, this is weird, as setting headers via Fetch is not a problem. You can keep it as is here, but don't use such a pattern in the real-world project. There's no reason to.

---

client/app/bundles/comments/rescript/ReScriptShow.res line 7 at r10 (raw file):

  commentStoreError: bool,
  saving: savingStatus,
}

Did you read the article I linked earlier regarding an illegal state?

[Yassa-hue]

Reviewable status: 66 of 69 files reviewed, 28 unresolved discussions (waiting on @alex35mil, @justin808, and @mightystrong)

---

client/app/bundles/comments/rescript/bindings/Axios.res line 9 at r10 (raw file):

Previously, alex35mil (Alex) wrote…

It is unrelated to axios, so it should be moved elsewhere.

I don't think we should use a record with a specific type in Axios binding at all. the data should be an object with an undetermined number of keys and values, and so the request options.

---

client/app/bundles/comments/rescript/Header/Header.bs.mjs line 95 at r10 (raw file):

Previously, alex35mil (Alex) wrote…

Remove.

done

---

client/app/bundles/comments/src/ReScriptShow.res line 4 at r7 (raw file):

Previously, Yassa-hue (Yassa Tamer) wrote…

Why Types module?

Because this type is needed in multiple files. When I tried to put it on the RescriptShow.res and call it in CommentForm.res I faced a dependency cycle error. My idea is to use a file that contains all types that are used in several files in the Types.res file. I agree the naming is not the best we can do. I suggest making a types file for the ReScriptShow.res component and putting on it all the types we need. We can call it a local type then.

Regarding this line of code specifically:
I don't think the form needs to know the status of the saving anyway it just needs a bool value to indicate if it should be disabled or not so I'll create the local type as you suggested and pass only a bool value disabled to the form.

I removed all the types files. I used local types.

---

client/app/bundles/comments/src/Actions/Actions.res line 38 at r7 (raw file):

Previously, alex35mil (Alex) wrote…

Regardless, this is weird, as setting headers via Fetch is not a problem. You can keep it as is here, but don't use such a pattern in the real-world project. There's no reason to.

NOTED!
I'll create an issue to use the same function for get and post requests in SimpleReactPage and Rescript page.

---

client/app/bundles/comments/rescript/ReScriptShow.res line 7 at r10 (raw file):

Previously, alex35mil (Alex) wrote…

Did you read the article I linked earlier regarding an illegal state?

Done.
We agreed on the implementation plan on slack.

[alex35mil]

Reviewed 4 of 4 files at r11, all commit messages.
Reviewable status: 66 of 69 files reviewed, 30 unresolved discussions (waiting on @justin808, @mightystrong, and @Yassa-hue)

---

client/app/bundles/comments/rescript/ReScriptShow.res line 5 at r11 (raw file):

type commentsFetchStatus =
  | FetchError
  | CommentsFetched(Actions.Fetch.comments)

It should be a single type.

State:

{
  commentsFetchStatus: NotStarted,
  commentsStoreStatus: CommentsFetched(comments),
}

is invalid, but still possible with the current design.

---

client/app/bundles/comments/rescript/ReScriptShow.res line 94 at r11 (raw file):

        disabled={switch state.commentsStoreStatus {
        | BusyLoading => true
        | _ => false

Don't use passthrough. It invalidates the benefits of exhaustiveness.

---

client/app/bundles/comments/rescript/ReScriptShow.res line 98 at r11 (raw file):

        storeCommentError={switch state.commentsStoreStatus {
        | StoreError => true
        | _ => false

Same.

---

client/app/bundles/comments/rescript/ReScriptShow.res line 106 at r11 (raw file):

        comments={switch state.commentsFetchStatus {
        | CommentsFetched(comments) => comments
        | _ => []

Same

---

client/app/bundles/comments/rescript/ReScriptShow.res line 110 at r11 (raw file):

        fetchCommentsError={switch state.commentsFetchStatus {
        | FetchError => true
        | _ => false

Same.

[Yassa-hue]

Reviewable status: 66 of 69 files reviewed, 30 unresolved discussions (waiting on @alex35mil, @justin808, and @mightystrong)

---

client/app/bundles/comments/rescript/ReScriptShow.res line 5 at r11 (raw file):

Previously, alex35mil (Alex) wrote…

It should be a single type.

State:

{
  commentsFetchStatus: NotStarted,
  commentsStoreStatus: CommentsFetched(comments),
}

is invalid, but still possible with the current design.

How is that possible?! When I tried the state you provided it didn't compile:

Code snippet:

  32 ┆ reducer,
  33 ┆ {
  34 ┆   commentsFetchStatus: NotStarted,
  35 ┆   commentsStoreStatus: CommentsFetched(comments),
  36 ┆ },

  This variant expression is expected to have type commentsFetchStatus
  The constructor NotStarted does not belong to type commentsFetchStatus

FAILED: cannot make progress due to previous errors.
>>>> Finish compiling(exit: 1)

---

client/app/bundles/comments/rescript/ReScriptShow.res line 94 at r11 (raw file):

Previously, alex35mil (Alex) wrote…

Don't use passthrough. It invalidates the benefits of exhaustiveness.

Done.

---

client/app/bundles/comments/rescript/ReScriptShow.res line 98 at r11 (raw file):

Previously, alex35mil (Alex) wrote…

Same.

Done.

---

client/app/bundles/comments/rescript/ReScriptShow.res line 106 at r11 (raw file):

Previously, alex35mil (Alex) wrote…

Same

Done.

---

client/app/bundles/comments/rescript/ReScriptShow.res line 110 at r11 (raw file):

Previously, alex35mil (Alex) wrote…

Same.

Done.

[alex35mil]

Reviewed 3 of 3 files at r12, all commit messages.
Reviewable status: 66 of 69 files reviewed, 27 unresolved discussions (waiting on @justin808, @mightystrong, and @Yassa-hue)

---

client/app/bundles/comments/rescript/CommentForm/CommentForm.res line 3 at r12 (raw file):

type formDisplay = Horizontal | Inline | Stacked

type commentsStoreStatus = Idle | BusyLoading | StoreError

Rename it to status or savingStatus.

type status = 
  | Idle
  | Saving
  | Error

[Yassa-hue]

Reviewable status: 66 of 69 files reviewed, 27 unresolved discussions (waiting on @alex35mil, @justin808, and @mightystrong)

---

client/app/bundles/comments/rescript/CommentForm/CommentForm.res line 3 at r12 (raw file):

Previously, alex35mil (Alex) wrote…

Rename it to status or savingStatus.

type status = 
  | Idle
  | Saving
  | Error

Done!

[alex35mil]

Looking good.

Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: 66 of 69 files reviewed, 26 unresolved discussions (waiting on @justin808, @mightystrong, and @Yassa-hue)

[Yassa-hue]

@justin808 I think the failing tests are fake faluires. The tests pass on the local run.