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.

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

[Feature request] Integrate with @deriving({abstract: light}) #183

Open
mvaled opened this issue Sep 27, 2022 · 13 comments
Open

[Feature request] Integrate with @deriving({abstract: light}) #183

mvaled opened this issue Sep 27, 2022 · 13 comments

Comments

@mvaled
Copy link

mvaled commented Sep 27, 2022

All the fields in the following script are reported as dead:

@deriving({abstract: light})
type person = {
  name: string,
  age: int,
}

let joe = person(~name="Joe", ~age=20)
let joeName = name(joe)

Report:

  Warning Dead Type
  File "src/TestBug.res", line 3, characters 2-14
  person.name is a record label never used to read a value
  <-- line 3
    @dead("person.name") name: string,

  Warning Dead Type
  File "src/TestBug.res", line 4, characters 2-10
  person.age is a record label never used to read a value
  <-- line 4
    @dead("person.age") age: int,

However, only age is not actually used in the code, while name shouldn't be reported.

@cristianoc
Copy link
Collaborator

Deriving abstract is expected to be used less and less in future. Either objects, or records, in the latest compiler versions, should be able to express all the needed functionality.

@mvaled
Copy link
Author

mvaled commented Sep 27, 2022

We use it heavily in a custom integration we have of a React-like framework based on Mithril -- this is mainly because our app is offline-first and the connection is very limited; so we wanted a really small library.

This, is for instance a very small component:

open Prelude
open Mithril
open CompletionStatus

@deriving({abstract: light})
type makeProps = {
  @optional completion: completionInformation,
  @optional children: element,
}

let make = _ => {
  component()->view(vnode => {
    let completion = vnode.attrs->completion->default(CompletionStatus.Defaults.completion)
    let {totalNested, totalInspected, isOverdue} = completion

    <div className="tile-action">
      {show(
        if completion->isFullyInspected {
          <Feather icon=#check_circle className="text-success" />
        } else {
          <span className={isOverdue ? "text-error" : ""}>
            {show(totalInspected->Int.toString ++ "/" ++ totalNested->Int.toString)}
          </span>
        },
      )}
    </div>
  })
}

And it can be used like <ShowCompletionInfo completion={...}/>.


It would require some rethinking to remove the dependency on abstract.

@cristianoc
Copy link
Collaborator

@mattdamon108 this seems doable with JSX V4?

@mununki
Copy link
Member

mununki commented Sep 27, 2022

@mattdamon108 this seems doable with JSX V4?

Yes, I think so too.

@mvaled Can you try the latest compiler and rescript-react as posted on the forum? https://forum.rescript-lang.org/t/call-for-help-2-test-jsx-v4/3781
Maybe something like:

type props<'completion, 'children> = {
  completion: 'completion,
  children: 'children
}
let make = props => {
  let {completion, children} = props

  component()->view(vnode => {
    let completion = vnode.attrs->completion->default(CompletionStatus.Defaults.completion)
    let {totalNested, totalInspected, isOverdue} = completion

    <div className="tile-action">
      {show(
        if completion->isFullyInspected {
          <Feather icon=#check_circle className="text-success" />
        } else {
          <span className={isOverdue ? "text-error" : ""}>
            {show(totalInspected->Int.toString ++ "/" ++ totalNested->Int.toString)}
          </span>
        },
      )}
    </div>
  })
}

@mununki
Copy link
Member

mununki commented Sep 27, 2022

If it is not working then try this:

type props = {
  completion: completionInformation,
  children: element
}

@mvaled
Copy link
Author

mvaled commented Sep 27, 2022

@mattdamon108

@mvaled Can you try the latest compiler and rescript-react as posted on the forum? https://forum.rescript-lang.org/t/call-for-help-2-test-jsx-v4/3781

Thanks.

One question, though. Would this require the actual React JS library or it's just the transformation, that would work with any compatible "react" binding? e.g Our rescript-mithril instead of '@rescript/react'.

If it's just the transformation, then I'll try to upgrade our Mithril bindings to be compatible with JSX v4.

@mununki
Copy link
Member

mununki commented Sep 27, 2022

I just checked about Mithril, never knew before. You made your own bindings!
How about your bsconfig.json? Do you use "reason"."react-jsx" option?

@mununki
Copy link
Member

mununki commented Sep 27, 2022

I've looked into your bindings. I think it would be working highly possibility. You can check the latest rescript-react binding and update your Mithrill bindings accordingly, then try with latest compiler's JSX v4.

@mununki
Copy link
Member

mununki commented Sep 27, 2022

One question, though. Would this require the actual React JS library or it's just the transformation, that would work with any compatible "react" binding?

Just transformation and working with react binding.

@mvaled
Copy link
Author

mvaled commented Sep 27, 2022

How about your bsconfig.json? Do you use "reason"."react-jsx" option?

None, actually. This is our whole bsconfig.json:

{
  "name": "kaiko-survey-tool",
  "sources": [
    {
      "dir": "src/",
      "subdirs": true
    }
  ],
  "suffix": ".js",
  "reason": {
    "react-jsx": 3
  },
  "bs-dependencies": [
    "@kaiko/reindexed",
    "rescript-webapi",
    "@ryyppy/rescript-promise",
    "rescript-mithril",
    "bs-fetch",
    "rescript-prelude",
    "rescript-deser",
    "rescript-action-queue"
  ],
  "package-specs": {
    "module": "es6"
  },
  "warnings": {
    "error": "+8+11+26+27+33+56"
  }
}

@mununki
Copy link
Member

mununki commented Sep 27, 2022

bsconfig seems you're using jsx transformation already. Yes, it's good sign. Good luck!

@mununki
Copy link
Member

mununki commented Sep 27, 2022

When you try the latest compiler with JSX v4, change this property in bsconfig.json

"reason": {
    "react-jsx": 3
  },

to

"jsx": {
  "version": 4
}

@mvaled
Copy link
Author

mvaled commented Sep 27, 2022

bsconfig seems you're using jsx transformation already

Right! Somehow my eyes skip over those lines 😄

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

No branches or pull requests

3 participants