Adapt example tests to Rescript11 + uncurried + RescriptCore #735
Conversation
| @@ -31,7 +31,7 @@ let make: (~hintSize: int, ~id: id<'key, 'id>) => t<'key, 'value, 'id> | |||
| `make(~hintSize=10, ~id)` creates a new map by taking in the comparator and `hintSize`. | |||
|
|
|||
| ```res example | |||
| module IntHash = Belt.Id.MakeHashable({ | |||
| module IntHash = Belt.Id.MakeHashableU({ | |||
There was a problem hiding this comment.
cc @cristianoc is this needed? This isn't pre-compiled into uncurried via MakeHashableU?
There was a problem hiding this comment.
No the compiler does not come with 2 versions of Belt.
Having to mention a xxxU function in documentation is pretty much a red flag, in that it instantaneously introduces tech debt.
There won't be any xxxU functions in future, so this is a strange point in time to document their use.
@zth do you have any thoughts about the solution here? The thing is, Belt will beed to be replaced with a library that lives outside the compiler, or in any case alongside RescriptCore. What we are witnessing here is simply that this has not happened yet.
There was a problem hiding this comment.
The mid to long term idea for Belt is to be a niche lib living outside of the compiler. This is explicit documentation for Belt, and Belt will be much less needed as Core "takes over".
I'm not sure I have a point here, more an observation. I think we can break Belt out to its own package as soon as we've shipped v11. I guess that would alleviate this particular issue.
Another observation is that Belt will be much less prevalent in docs and examples now that Core exists and we're moving everything to that. So this might not be such a big problem going forward as we think.
There was a problem hiding this comment.
According to @cknitt getting rid of Belt in the compiler is no small feat, but I don't know the full details.
There was a problem hiding this comment.
Belt in the compiler can be considered an implementation detail, that can be addressed over time.
I think we can restrict attention to user visible docs for this.
So I guess we can proceed and make a note about next steps in a separate issue.
There was a problem hiding this comment.
@cristianoc I already started a few attempts to remove Belt from the compiler, but never finished those.
There are two problems:
- Pervasives defining stuff based on Belt (
type result<'a, 'b> = Belt.Result.t<'a, 'b>). - Belt being used extensively in various tests.
Are you suggesting an approach like the following:
- Reverse the dependency from Pervasives to Belt.
- Copy Belt into a separate repo and get it to build as a normal ReScript project (this should be rather easy). Publish to npm as
@rescript/belt. - Keep Belt in the compiler repo for now so that the tests still work, but do not publish any Belt stuff as part of the
rescriptnpm package anymore, having users use the separate package instead. - Clean up Belt usage in the compiler repo over time.
?
There was a problem hiding this comment.
Yes something like this sounds like a good approach.
Though I would try to make the compiler's result, not Belt, the source of truth already now.
As otherwise that's going to pop up somewhere, be it hovering in the editor, or some error message.
There was a problem hiding this comment.
| @@ -68,7 +68,7 @@ Regex literals `%re("/.../")` should generally be preferred, but `fromString` is | |||
| let firstReScriptFileExtension = (filename, content) => { | |||
| let result = Js.Re.fromString(filename ++ "\.(res|resi)")->Js.Re.exec_(content) | |||
| switch result { | |||
| | Some(r) => Js.Nullable.toOption(Js.Re.captures(r)[1]) | |||
| | Some(r) => Js.Re.captures(r)[1] | |||
There was a problem hiding this comment.
This isn't in Core right, still need to access Js.Re?
| Js.log("Found " ++ (match_ ++ (". Next match starts at " ++ next))) | ||
| }) | ||
| | Some(result) => | ||
| Js.Nullable.iter( |
There was a problem hiding this comment.
Is there an equivalent in the global Nullable module?
| Js.Nullable.iter( | ||
| Js.Re.captures(result)[0]->Js.Nullable.fromOption, | ||
| (. match_) => { | ||
| let next = Belt.Int.toString(Js.Re.lastIndex(re)) |
| let matchStr = | ||
| match_->Js.Nullable.toOption->Belt.Option.getWithDefault("") |
There was a problem hiding this comment.
This could be a regular match on the Nullable.t, right?
| let next = Belt.Int.toString(Js.Re.lastIndex(re)) | ||
| let matchStr = | ||
| match_->Js.Nullable.toOption->Belt.Option.getWithDefault("") | ||
| Js.log("Found " ++ (matchStr ++ (". Next match starts at " ++ next))) |
|
So I tried to make the tests work again which is why I adapted the API examples only to the point that they compile again. Which is why I did not adapt the API examples to use RescriptCore. But this state is completely weird and I also don't know how to handle it well. |
fhammerschmidt
force-pushed
the
rescript-11
branch
from
34e6f2f to
7cfb4ad
Compare
fhammerschmidt
force-pushed
the
rescript-11
branch
from
01b9df8 to
9d689e2
Compare
|
@zth Tests are passing now, let's merge this one. As I said before, I left the API docs relatively untouched besides fixing the tests. The rest of the docs is more important now. |
|
Go for it! |
…t-lang#735) * Adapt some pages to ReScript 11 including records and variants * Some better wordings * Adapt example tests to Rescript11 + uncurried + RescriptCore * Change more examples from Belt to RescriptCore * Fix uncurried error * Consistent quotes * Fix remaining test * Mention RescriptCore in try section * Add dummy file
No description provided.