Rescript-compiler: Matching Js argument order with Belt

[rickyvetter]

Currently Belt and the Js modules have the opposite argument order: Js.Array.map(fn, array), Belt.Array.map(array, fn). I've noticed some pain in having to switch between the two and was wondering how people felt about moving Js.X functions to the argument order that matches Belt for consistency.

Procedure for converting any module X:

Minor version bump:

Introduce Js.X2 with the new order, deprecate all functions in Js.X, provide a codemod from X to X2.

Major version bump:

Replace Js.X with the contents of Js.X2 and remove Js.X2**. Provide a codemod from X2 to X (this one is very easy - just mechanical).

** alternatively we could do this a bit slower and deprecate X2 and in a future major version remove it.

How do people feel about this? Could execution be better?

[bobzhang]

Js.X2 sounds good to me

[glennsl]

Mind if I point out (again) how completely fucking ridiculous it is to make such a massive ecosystem-wide change and break from a well-established functional programming convention just for some marginal performance gain and slightly better type inference in a few rare cases? This problem was foreseen, but ignored, and won't go away until the entire ecosystem has converted anyway. Which I doubt will ever happen. Instead it'll likely just cause more fragmentation.

The correct path forward is to undo the stupidity, not to double down on it.

Having said that, as a library maintainer (and probably as a user if I still used it) I find the current procedure in bs-platform releases of immediately deprecating old APIs when adding something new to be extremely annoying, since warnings from even just using it in library internals get exposed to end users. And since the only way to fix this is to use the new API and thus break with older versions of bs-platform, it forces a wave of major releases up the dependency chain.

And of course there's never prior notification of these deprecations, so it all comes as a surprise on the day the new bs-platform is released, usually in the form of an issue submitted by an end-user being sufficiently annoyed by all the warnings.

It is perhaps more of an issue with many small deprecations than one big one, but this is still going to a cause a formidable sea of warnings that will stick around for a long time unless it's addressed. Perhaps it's possible to suppress deprecation warnings only when building dependencies?

[rickyvetter]

Hey @glennsl - I didn't mean for this to turn into a debate about the merits of any given argument order. I recognize that there isn't a view shared by everyone in the community. The end goal of this issue is unification of the api and the exact same deprecation would need to happen if we were to update in the other direction.

Perhaps it's possible to suppress deprecation warnings only when building dependencies

@bobzhang is this something we can do?

there's never prior notification of these deprecations

Do you have recommendations for notice? Aside from this issue I thought I would post in the discord channel. Maybe a blog post ahead of time would be warranted?

[glennsl]

I didn't mean for this to turn into a debate about the merits of any given argument order. I recognize that there isn't a view shared by everyone in the community.

Don't you think it's a bit disingenuous to dismiss this as just a difference of opinion within the community, when the community was largely ignored? Had it not been, I would of course have preferred to sort this out in the original issue or PR, before it was forced through. Alas, now we are here...

Do you have recommendations for notice? Aside from this issue I thought I would post in the discord channel. Maybe a blog post ahead of time would be warranted?

I don't follow along on Discord anymore, and found it hard to keep up even when I was an active user. I'm much more likely to notice a forum post.

It would be really cool if there was a script that crawled every repository listed in the redex database for deprecations and created an issue notifying the maintainer of it. Or better yet, a PR with the codemod applied! Could be a good beginner project for someone looking to start contributing.

[bobzhang]

@rickyvetter there is an issue #2803, but the purpose of deprecating is to nudge people to change it as soon as possible, so I am unsure it is a good idea or not

[ostera]

@rickyvetter for reference, I opened a PR some time ago to start talking about standard interfaces that would address these issues: just have everyone follow the same interface and we don't need to talk about different argument ordering.

[phated]

I just stumbled onto this issue (and it seems like too late to effect anything since @chenglou's PRs were all accepted) but why can't things exist in parallel? Instead of the poor naming of Js.Promise2, etc (I had absolutely no idea why those PRs were happening), why not just put them into a Js.PipeFirst namespace, so you can use Js.PipeFirst.Promise, etc.

[chenglou]

We're not shipping Array2, TypedArray2 and others just yet. These are PRs whose naming can be tweaked in the future.
We don't want a PipeFirst module; even if we do, you wouldn't use Js.PipeFirst.Promise; you'd likely alias the module. Which you'd do for Promise2 anyway (again, we're not shipping it yet).
We need to establish an idiom for the community to deprecate old APIs and move toward new ones. So for example, if you wanna change the signature/semantics of a function called foo, you'd change it to, let's say, foo_ or foo2 or foo'. This'll indicate to the consumer that you intend to swap back to foo later. It's a two-steps upgrade process, one that a type system helps a lot with. Obviously, directly changing foo would be a breaking change, but foo -> foo_ -> foo wouldn't be unless you skip a long version.

[ELLIOTTCABLE]

Er, that's still a breaking change.

It doesn't matter how many version numbers you sneak in-between; code that has one expectation of the meaning of Foo.bar is going to continue working when you introduce Foo.bar2 (thus, not a breaking change) … and break when you change Foo.bar (thus, a breaking change) to match. It doesn't matter how many intermediary names you introduce, it's still a breaking change when the deprecation finally lands.

Like, you can't dance around breaking changes, if you want to change existing APIs. You're stuck with them. I'm not sure how this is non-obvious or surprising to anybody involved?

This seems like a really strange way to handle a well-known, well-studied, and provably-unsolvable problem. Literally, the solution is not naming, it's documentation (which, by the way, I saw none of when I was trying to google why the hell the docs now have two Js.Array modules, and which one I should use …).

(I'm sorry if this seems aggressive, I'm just very confused by the choices being made here!)

[chenglou]

This indeed seems a bit aggressive, especially when we're not sneaking in anything, and when we know very well what the definition of a breaking change is.

Changing directly from one signature of foo to another signature of foo means you need to upgrade your codebase at once onto the new API, which is unrealistic for any non-trivial codebase; you can't even your split changes into multiple non-atomic diffs. Preserving foo and introducing a foo2 allows you to migrate even line by line if you wish, to the new api, while keeping parts of codebase still on foo. With an interop semantic of exactly nothing, because in this context, your JS output isn't affected since it's all externals. After a while, when we know that userland is mostly done with the migration, we change foo's semantics to match foo2, which is, yes, a breaking change, but one that hopefully no longer affects most parties because they've all progressively migrated to foo2. Then you can, again, progressively migrate back to foo just for the better naming, once again with no JS output change. This strategy gives you a two-steps _progressive_ migration versus directly changing foo's semantics and hoping that you can one-shot convert the codebase and land the diff, which isn't possible if you're relying on third-party that's still on the old foo; you'd be now waiting on that third-party to migrate first (atomically), before migrating yourself. This is even worse if said third-party is a transitive dependency; you wouldn't even be able to fork it if there's a diamond dependency shape.

A two-step migration like such is a normal way to handle the well-known, well-studied problem. The solution is precisely new names (and in this case, said naming is the equivalent of creating a new namespace, aka treating the old one as immutable and structurally share bindings, if we wanna throw around fancy concepts). Though practically speaking, nobody wants to be stuck with Foo.Version20.bla forever (this needs some package-level support that e.g. Go is experimenting), thus the second step of the process to move things back into their original canonical names.

We haven't documented the new modules precisely because they're not stable yet; in general, we do take the position that "if they're not documented, they don't exist". This is a healthy, responsible stance for the community in general to adopt. So if it's not documented, it's internal implementation details that we're trying out in the open for proper procedure. In this case, we want to dog food the module changes before exposing them publicly.

[ELLIOTTCABLE]

So the crux of my problem here is 100% the documentation.

“if they're not documented, they don't exist” — if I google "BuckleScript Array" right now, basically the first result is this page, on which the top example is the following:

[| 1;2;3;4|]
|. Js.Array2.map (fun x -> x + 1 )
|. Js.Array2.reduce (+) 0
|. Js.log

Again, I'm not making this up — the only reason I've found this issue, is because I, personally, experienced exactly this confusion: acting as a user, using the direct-binding types instead of OCaml types (not my usual use-case, so one I don't know off the top of my head), I googled that, and was presented (without explanation) something about ‘Array2.’

Perhaps this needs a new Issue, then, as to why experimental, upcoming, not-fully-decided changes are literally at the top of the only existing user-facing documentation? Perhaps https://bucklescript.github.io/bucklescript/api/ shouldn't track master; or (and?) should have an explicit version-navigation heading. I don't know, but I'm happy to help with that, if you're short man-hours, and nobody else is interested in improving the new-user experience.

Regarding the rest — I still disagee, strongly; but at this point, it's he-said/she-said about what's ‘accepted practice.’ From my perspective, and I understand that you don't agree, a deprecation warning being broadcast to all users at vB.B.B, saying “Foo is deprecated and will be replaced soon, please switch to Foo2 and change your argument order!”, is no different from that same warning being broadcast six months earlier, on vA.A.A, saying “Foo is being deprecated soon, please switch your argument order!”

tl;dr I see two failures here, and I think most of your reply focuses on the second, smaller one — how exactly something is named. It's easy to agree with everyone else in the thread, there: we have strong refactoring tools! The much bigger failure is in documentation and communication, though, y'know? If you're not settled on “Array2”, then Array2 should be showing up literally nowhere that's user-facing; I don't understand why that's a controversial opinion. 😨

[yawaramin]

@ELLIOTTCABLE

Perhaps https://bucklescript.github.io/bucklescript/api/ shouldn't track master; or (and?) should have an explicit version-navigation heading.

This is pretty much it, isn't it? People are getting confused because the docs are showing stuff that's in master and not ready for release yet. It pretty much boils down to not having a good documentation situation for OCaml. Everything else we can work around–deprecations and breakages with codemods and overlay modules, e.g. But lack of good docs is systemic.

I'm happy to help with that, if you're short man-hours, and nobody else is interested in improving the new-user experience.

So let's discuss what we can do about documentation. As far as I can tell, @ostera and maybe others are working on it but maybe we should chime in with them and see if we can help there.

[bobzhang]

Perhaps https://bucklescript.github.io/bucklescript/api/ shouldn't track master; or (and?) should have an explicit version-navigation heading.

Yes, we should have docs per version.
We just need publish docs for each release instead of only one coy at this time

[cknitt]

@bobzhang @rickyvetter @chenglou When do you envision the switchover from Js.X2 to Js.X to happen? The current situation is still extremely confusing for newcomers...

I looked through all the submodules of Js, and as far as I understand, the current status is:

Already pipe first / no need for an X2 module:

• Js.Console
• Js.Date
• Js.Dict
• Js.Exn
• Js.Float
• Js.Global
• Js.Int
• Js.Json
• Js.Math
• Js.Null
• Js.Null_undefined
• Js.Nullable
• Js.Obj
• Js.Result
• Js.Types
• Js.Undefined

Conversion status of pipe last modules:

• [x] Js.Array -> Js.Array2
• [ ] Js.List
• [ ] Js.MapperRt
• [ ] Js.Option
• [ ] Js.Promise
• [x] Js.String -> Js.String2
• [x] Js.Typed_array -> Js.TypedArray2
• [ ] Js.Vector

IMHO there is no reason why someone would use Js.List or Js.Option over Belt.List / Belt.Option. So these could be deprecated and moved into a JsCompat module or something like that.

Ignoring Js.MapperRt, which seems to be some internal stuff, this actually leaves us with just Js.Promise and Js.Vector to convert to pipe first, right?

[fhammerschmidt]

ReScript would be an opportunity to clean this up.