search

TheSpyder / rescript-webapi

ReScript bindings to the DOM and other Web APIs
http://tinymce.github.io/rescript-webapi/api/Webapi/
Other
151 stars 37 forks source link

Improve tests with expected types and ensuring every line generates code #42

Open TheSpyder opened 3 years ago

TheSpyder commented 3 years ago

There are two problems with our tests:

  1. They use let _ = blah() which means if the type of a method changes the test still compiles. This is not good.
  2. By always using _ to avoid exporting the reference, some lines are optimised away by ReScript and never emitted (this is arguably a bug, but it's what we are stuck with for now).

To solve the first problem, change things like

let _ = range->getClientRects

to

let _: array<Dom.domRect> = range->getClientRects

For the second, about all we can do is audit the JS and see which lines don't result in compiled output. I have found that externals returning an optional value in a let _ = line are not emitted, here's how I solved that: https://github.com/tinymce/rescript-webapi/blob/b138ae2982d24193473e01929251871ea2d4ba65/tests/Webapi/Dom/Webapi__Dom__Document__test.res#L64-L67

The more I think about this the more I believe it should be logged as a bug with ReScript. But we should find them all first to make the bug report more valuable.

spocke commented 2 years ago

I made them export a reference in a PR so I changed let _ = range->getClientRects to let rects = range->getClientRects that way the compiler will not throw them away I don't mind it adding a export to the test module no one will include that directly anyway we just make sure they are not shipped in the npm package.

So maybe we switch it to this: let clientRects: array<Dom.domRect> = range->getClientRects ?

spocke commented 2 years ago

I updated a PR I made to use that approach I think it's short and easy to read. https://github.com/tinymce/rescript-webapi/pull/48/files

It adds a bit of code to the generated js files for the exports but not having to boilerplate code for every property/function we test would be nice.

spocke commented 2 years ago

Noticed a different variant with let test_foo_bar =.. not sure I like snake case for rescript things.

TheSpyder commented 2 years ago

I have started a branch for this, there's a lot to do, but I've nearly covered the 10 files in the top-level webapi folder. I'm not looking forward to updating the dom folder 😂