Were bs.deriving(jsConverter) and bs.deriving(abstract) removed?

1

In an unrelated package update, my CI started failing over these two decorators. It looks like the abstract isn’t needed anymore, but the jsConverter was handy for quickly converting a type to/from an int in JSON.

Exceptional admin edit: no it hasn’t.

2

These two deccorators should still work afaik. See this playground.

What’s the complete error message you are getting? And which rescript version are you on?

3

Hrm, that’s strange. I’m on bs-platform@9.2. Oddly the upgrade that broke this was moving to the latest graphql-ppx. But I had the same problem locally as well.

FAILED: src/bindings/ReactHelmet.ast

  We've found a bug for you!
  /home/runner/work/ido/ido/src/bindings/ReactHelmet.res:1:11-18
  
  1 │ @deriving(abstract)
  2 │ type htmlAttributes = {lang: string}
  3 │ type bodyAttributes
  
  Ppxlib.Deriving: 'abstract' is not a supported type deriving generator


  We've found a bug for you!
  /home/runner/work/ido/ido/src/components/TripNormalView.res:12:11-21
  
  10 │ ]
  11 │ 
  12 │ @deriving(jsConverter)
  13 │ type elementView =
  14 │   | ListView
  
  Ppxlib.Deriving: 'jsConverter' is not a supported type deriving generator
5

Related issue. Seems like ppxlib doesn’t mix well with the builtin decorators. Probably gonna need some more discussions to fix this properly, unfortunately.

6

More discussions… with a locked thread in the issue? How convenient.

7

Most of us were unaware of this until just now. Let’s keep the discussion here until we can resolve that issue!

1 Like
8

Ouch yeah- that’s exactly the problem I hit that jfrolich even identified. My sleuthing sadly didn’t take me to the Rescript Compiler project, so that’s not a very discoverable location.

So is there a workaround? I think my code is fine with dropping the few instances where I was using @deriving, but losing jsConverter is annoying- that’s a great little helper.

9

Not entirely sure, but i think you’d need to downgrade to an older graphql-ppx version for now… your issues came up with the latest upgrade, right?

10

Yes, I can just not upgrade it- the PR’s still waiting. Is there a plan to somehow resolve this in the future? The linked ticket was closed out pretty quickly so I’m not sure what the official recommendation is.

11

Thanks to @fakenickels I found a fix to let ppxlib not raise this error. So it was easier solved than I thought initially! I’ll release this today as graphql-ppx 1.1.1.

Finding a fix like this was exactly why I opened an issue, so not sure why the issue was closed on GitHub and not sure where the OCaml/JSOO comment comes from and why the discussion was locked. It was probably just misinterpreted by @Hongbo. I could probably have formulated the issue better.

11 Likes
12

@jfrolich, you’re a star. Thanks for working through this with patience and grace.

5 Likes
13

I also created a PR to styled-ppx to solve this issue there.

4 Likes
14

Hi, let me explain why that issue is locked, I apologize that you are affected.

First, the topic itself is indeed something we won’t fix. Actually I made a post here two weeks ago Suggestions of naming conventions for third party ppx attributes . All third party attributes are encouraged to be qualified, the built-in attributes are not qualified, this is to allow us emit warnings for unused attributes. And I think it is repeated again and again, rescript is its own language.

Second, OSS is free service, we spend our time including weekends sharing something that we think is valuable, we ask for nothing but we want to be happy. Japp(the creator of the issue) is actively working on something which is hurting the ecosystem (he may think not) – it is perfectly fine, but I do think we have the option to decide how to spend our time. If he decides to contribute to our vision, he is welcome, if he is not, we wish him the best, but we just want to stay quiet and focus on our own tuff.

15

@Hongbo, can you be more elaborate on this? You think graphql-ppx is hurting the ecosystem? What is your proposed alternative?

I can’t imagine being productive with GraphQL without something like graphql-ppx.

16

I think you know what I mean. It’s okay to disagree & I wish you all the best.
For people who are reading the thread, it’s really not easy work to manage a community , I wish I could have your sympathy, we just want to stay quiet and focus on our own stuff

17

I honestly don’t know what you mean here. If you don’t want to say it publicly my DM’s are open.

I’ve always tried to be supportive. Sometimes I probably have had some feedback opposing some features given my experience as a user, when I thought that feedback might be valuable to the core-team, but I am fine if you discard it.

1 Like
18

Hoooold. Let’s not accidentally misinterpret words here. Graphql is a fine use-case. Facebook created it after all.

There are several ways of implementing it. These technicalities, we can discuss better in various dedicated threads.

However, in general, ppx are discouraged, for both the aforementioned reasons and more. We do make a few exceptions here and there when the ROI is large, and graphql might count.

19

Completely agree. Ppx’s are not perfect indeed. Good to know rescript is still supportive when there is no better solution.

I just want reiterate, the issue had nothing to do with arguing for @deriving to be supported in the OCaml way, it was just an issue that some ppx’s that adopted ppxlib were breaking under the newest rescript release. So it seems like @Hongbo is still misunderstanding the issue.

We found a solution, but it would be better to have the discussion in the issue IMO and not lock it down entirely. This also has nothing to do with supporting the fork. ppxlib has been merged before there was a fork in some ppx’s, and it’s predecessor (ocaml-migrate-parsetree) was implemented to facilitate the parse tree migration from 4.02 to 4.06 some time ago and maintain backwards compatibility. And it also helps us support ReasonML native (but even if we wouldn’t support that, we still would need to use ppxlib).

Anyway this is the last thing I will say about it. I think it’s important to have have empathy, and assume good faith.

2 Likes
20

For people who need ppx in some cases, we recommend not relying on ppxlib.
ppxlib is introduced to solve the ABI instability in the native land, while it is not an issue in rescript, the parsetree is quite stable across releases.
We suffered from omp for quite a while due to the refmt parser which bloated the size significantly, it is better to not repeat the same mistake.

1 Like
21

Ok this is interesting. Thanks for that. I didn’t know that as a ppx maintainer, and I am not aware of any ppx’s that do it this way currently. There are several issues with that from my standpoint:

  1. I have an m1 mac, and can’t compile old ocaml versions easily (with some emulation, but not preferable)
  2. Editor tooling doesn’t really work well with really old versions of OCaml
  3. I can still remember the move from 4.02 to 4.06 and it was quite a pain on the part of the ppx maintainer, because needing to ship two binaries (including CI pipeline) and having separate instructions depending on which version of bs-platform users were (you always have some transition period). This is happening again later this year.

So going for this other approach will have some downsides aside from the extra work and not a big upside (some extra performance will be nice, but I don’t have the capacity to trade-off a lot of extra maintenance for performance because it’s all voluntary work).

The problem is solved for now so I think we are ok!

5 Likes