Skip to content

Instantly share code, notes, and snippets.

@despairblue
Created May 27, 2018 21:46
Show Gist options
  • Save despairblue/643cf1af058d0f9dc95d49528f657d90 to your computer and use it in GitHub Desktop.
Save despairblue/643cf1af058d0f9dc95d49528f657d90 to your computer and use it in GitHub Desktop.
Reason Dojo Notes

Reason Dojo Notes

General Problems

  • wrapping components is repetetive, using all those @bs.deriving

    • [problem] No one of us read the documentation and with it looking like a record we expected it to behave like a record.
      • Why do we need to call Composable.projectionConfigT instead of just passing in a record?
      • Why is a type a function?
    • We should have checked the documentation, I just read it and now I understand it. In my opinion it should not look like a record if I cannot use it like one. Maybe the syntax can be changed. I don't know enough about PPX to know if that is possible though.
    • [solution] Find another syntax that does not imply that this is a record.
    • [solution] Do less autogeneration.
  • the interop has too much boilerplate

    • [problem] Every property has to be typed 3 times.

      module ComposableMap = {
        [@bs.deriving abstract]
        type projectionConfigT = {
          scale: int,
          rotation: array(int),
        };
        [@bs.deriving abstract]
        type jsProps = {
          projectionConfig: projectionConfigT,
          /* FIRST OCCURANCE */
          width: int,
          height: int,
          style: ReactDOMRe.Style.t,
        };
        [@bs.module "react-simple-maps"]
        external composableMap : ReasonReact.reactClass = "ComposableMap";
        /* SECOND OCCURANCE */
        let make = (~projectionConfig, ~width, ~height, ~style, children) =>
          ReasonReact.wrapJsForReason(
            ~reactClass=composableMap,
            /* THIRD OCCURANCE */
            ~props=jsProps(~projectionConfig, ~width, ~height, ~style),
            children,
          );
      };
    • [solution] You want to define the wrapping more declaratively (maybe a PPX).

    • [solution] ReasonReact.wrapJsForReason should take props type and return a make function (not sure what it takes to get this working or if it is possible at all):

      module ComposableMap = {
        [@bs.deriving abstract]
        type projectionConfigT = {
          scale: int,
          rotation: array(int),
        };
        [@bs.deriving abstract]
        type jsProps = {
          projectionConfig: projectionConfigT,
          width: int,
          height: int,
          style: ReactDOMRe.Style.t,
        };
        [@bs.module "react-simple-maps"]
        external composableMap : ReasonReact.reactClass = "ComposableMap";
        let make = ReasonReact.wrapJsForReason(
          ~reactClass=composableMap,
          ~props=jsProps,
        );
      };
  • reactjs JSX and reason JSX should be copy 'n pastable

    • People copy'n pasted the JSX from simple map example and expected it to work without changing a lot of syntax. I can ask for specifics if this is too general.
  • Belt's documentation could be improved

    • Hard to find.
      1. You search on google.
      2. Find and empty git repository.
      3. Follow the link to bucklescript.
      4. See that autogenerated documentation which most people don't even realize is documentation.
    • Standard format for documentation of reason/ocaml is hard to read, it looks so alien.
      • People don't realize that you need to click module names to traverse it.

Things that confused when looking at the solution for help

  • Using fastpipe for property access was confusing, it provided no syntactic improvement in that situation and confused people that where acqainted with the pipe operator: "Why use another pipe when the normal one would do the same job?"

    /* These are all equal, why not just use the last one? */
    let (lat, long) = marker |. Fetcher.location;
    let (lat, long) = marker |> Fetcher.location;
    let (lat, long) = Fetcher.location(marker);
  • SetState is a bad name for an action. People didn't understand that SetState is a message type for the reduce and that those can be name arbitrarly by the programmer, they thought that just the way you do this.setState() in reason, and assuming that it das look convoluted and like overkill.

  • open Belt shadows the native List and Array module leading to weird issues where merlin tells you a function exists (specifically Array.of_list and Array.to_list) but the compiler tells you it does not.

    • Shadowing variables/modules should give a warning in general.
    • open should never be used in example code. It makes everyone's live miserable and usage should not be encouraged in the comunity 😄
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment