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

Improve TS types #3

Merged
merged 16 commits into from
Jul 9, 2024
Merged

Improve TS types #3

merged 16 commits into from
Jul 9, 2024

Conversation

lishaduck
Copy link
Contributor

@lishaduck lishaduck commented Jul 7, 2024

Depends on #5
Here's the diff as GH doesn't play well with stacks from forks: lishaduck/elm-solve-deps-wasm@better-cargo+clippy...better-ts-types

Removes any from the generated TS types.
The way the hashmap was handled made the minimal diff, but a more "correct" way of doing it would use serde-wasm-bindgen to serialize it.
The commit to update parameters should 100% make no difference to the generated wasm (as should the typo fix)
Dependency updates and the return type changes might make some differences, but I didn't notice tests. How should I check it works?

Closes: #2

P.S.: Correct me if I'm wrong, but I think the main consumer of this is elm-review. If elm-review does migrate to ESM, I'll be back! 😄

@lishaduck
Copy link
Contributor Author

The way the hashmap was handled made the minimal diff, but a more "correct" way of doing it would use serde-wasm-bindgen to serialize it.

I take this back. I accidentally said it was a function. Fixing that broke some other stuff, so I'll probably go back down this road.

@lishaduck
Copy link
Contributor Author

The way the hashmap was handled made the minimal diff, but a more "correct" way of doing it would use serde-wasm-bindgen to serialize it.

I take this back. I accidentally said it was a function. Fixing that broke some other stuff, so I'll probably go back down this road.

I take that back as well 🤣 I forgot to save, so the diagnostics didn't go away. This code compiles fine.

@mpizenberg
Copy link
Owner

ahah you’re fast. So there are two main users of this, elm-review (the node package) and elm-test (the node package). So these are the two you should check with.

@lishaduck
Copy link
Contributor Author

ahah you’re fast. So there are two main users of this, elm-review (the node package) and elm-test (the node package). So these are the two you should check with.

For tests, you mean?

@mpizenberg
Copy link
Owner

For checking that nothing broke for them, and checking that they are fine with the type upgrade or if that messes things up with their setup.

@lishaduck
Copy link
Contributor Author

For checking that nothing broke for them, and checking that they are fine with the type upgrade or if that messes things up with their setup.

👍

I'm checking elm-review, as I made these changes by "copying" them over.
I'll check elm-test after that (might break tests if incorrect impls, but it's not typesafe).

@mpizenberg
Copy link
Owner

For testing, most of the logic testing is done in the core library. This package is just a wrapper to provide a wasm package. If you can check that the two examples (offline and online) works, that would be enough I think. In addition to getting @jfmengels and @lydell on board with the changes.

@lishaduck
Copy link
Contributor Author

lishaduck commented Jul 7, 2024

For testing, most of the logic testing is done in the core library. This package is just a wrapper to provide a wasm package.

Makes sense.

If you can check that the two examples (offline and online) works, that would be enough I think.

They work! (well, offline errors, but correctly.)

Logs

Online

Solution for test dependencies:
Offline solver failed, switching to online solver.
{
  direct: {
    'elm/core': '1.0.5',
    'elm/json': '1.1.3',
    'elm/random': '1.0.0',
    'elm/time': '1.0.0',
    'elm-explorations/test': '1.2.2',
    'ianmackenzie/elm-float-extra': '1.1.0',
    'ianmackenzie/elm-units': '2.10.0'
  },
  indirect: { 'elm/html': '1.0.0', 'elm/virtual-dom': '1.0.3' }
}

Offline

Directory "/Users/dukese01/.elm/0.19.1/packages/ianmackenzie/elm-float-extra does not exist
Not doing a request to the package server to find out existing versions. Please run at least once elm-test first.
Directory "/Users/dukese01/.elm/0.19.1/packages/ianmackenzie/elm-units does not exist
Not doing a request to the package server to find out existing versions. Please run at least once elm-test first.
Directory "/Users/dukese01/.elm/0.19.1/packages/ianmackenzie/elm-float-extra does not exist
Not doing a request to the package server to find out existing versions. Please run at least once elm-test first.

/Users/dukese01/Developer/elm-solve-deps-wasm/pkg/elm_solve_deps_wasm.js:252
            throw takeObject(r2);
            ^
Because there is no version of ianmackenzie/elm-float-extra in 1.1.0 <= v < 2.0.0 and ianmackenzie/elm-units-interval 2.3.0 depends on ianmackenzie/elm-float-extra 1.1.0 <= v < 2.0.0, ianmackenzie/elm-units-interval 2.3.0 is forbidden.
(Use `node --trace-uncaught ...` to show where the exception was thrown)

Node.js v20.12.2

In addition to getting @jfmengels and @lydell on board with the changes.

I can wait, elm-review's not blocked. 😁

P.S.: I mostly write Dart these days, and love that this uses PubGrub. Nice, familiar errors 😁

@lishaduck
Copy link
Contributor Author

Presumably you'd like if I break up this PR?
I'm thinking

  1. Update examples folder to a modern setup
  2. Cargo changes
  3. The changes to the .d.ts file generation

@lishaduck lishaduck mentioned this pull request Jul 8, 2024
lishaduck added 7 commits July 7, 2024 21:41
Apparently Prettier and flow comments don't like each other.
Thus, it's a ts project now :)
Just what cargo bumps automatically.
@lishaduck lishaduck force-pushed the better-ts-types branch 2 times, most recently from 976ecf8 to 5c6b752 Compare July 8, 2024 02:47
@lishaduck
Copy link
Contributor Author

I've uploaded this to github:lishaduck/elm-solve-deps-wasm-pkg for testing, as npm has an longstanding bug with local path dependencies.

lishaduck added 2 commits July 7, 2024 22:05
Good ol' clippy found a thing or two.
This is a high quality codebase—these lints were only added a version or two ago.
Copy link
Contributor Author

@lishaduck lishaduck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so all of elm-review's test suite passes with these changes: jfmengels/node-elm-review#195.

@lydell
Copy link

lydell commented Jul 8, 2024

node-test-runner unfortunately still uses Flow instead of TypeScript, so the change of types here does not affect it. Sounds good to improve the types, though!

@lishaduck
Copy link
Contributor Author

node-test-runner unfortunately still uses Flow instead of TypeScript, so the change of types here does not affect it.
Sounds good to improve the types, though!

Ok, thanks for the feedback. I still plan on testing though, as wasmbindgen has some newer features that allow these types, and so I've switched a few very minor minor impl details.

@lishaduck
Copy link
Contributor Author

Simon said that the tests pass for elm-test-runner, so I think that's the last external blocker, we just need to resolve the remaining questions in #5.

@lishaduck lishaduck force-pushed the better-ts-types branch 2 times, most recently from 58676e2 to 78152bf Compare July 8, 2024 23:10
lishaduck added 3 commits July 8, 2024 18:11
Tidy up the code a bit, as there're no Rust consumers of this package.
@mpizenberg
Copy link
Owner

I’m on a new machine (Mac M3) and was trying to recompile the project on the main branch first before checking out your changes. I’m not sure why, but I get this error while trying the online example:

image

I have already compiled with wasm-pack the rust lib, and the pkg/ directory is present at the correct location relative the the package.json file in the example folder. I’m not familiar enough with the JS ecosystem to know what this error is really about. Any idea?

@mpizenberg
Copy link
Owner

AH, I had just forgotten to npm install 🤦

@mpizenberg mpizenberg merged commit 333f815 into mpizenberg:main Jul 9, 2024
3 checks passed
@lishaduck lishaduck deleted the better-ts-types branch July 9, 2024 17:01
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

Successfully merging this pull request may close these issues.

3 participants