Skip to content

Update dependencies #105

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

Merged

Conversation

PascalHonegger
Copy link
Collaborator

No description provided.

Copy link

netlify bot commented Jan 4, 2025

👷 Deploy request for coronate accepted.

Name Link
🔨 Latest commit 84fd5b0
🔍 Latest deploy log https://app.netlify.com/sites/coronate/deploys/67d5fb4fa3f0d900082574ec

Pull requests and bug reports are welcome! */
type t
@get external getHelpers: numeral => t = "_"
// module Helpers = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Couldn't get these to compile at all, no idea why

Copy link
Owner

Choose a reason for hiding this comment

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

Ugh. This code was always pretty messy and I don't recall the details of how/why I designed it now. I can try to take a look at it later when I get a chance.

Instead of globally suppressing warning 44.
@johnridesabike
Copy link
Owner

After updating the test files, there seems to be some issues with the localforage code where it doesn't seem to work with the tests anymore. I'm not sure what broke or how to fix it, but honestly the whole test setup is probably one of the things that needs to be completely redone eventually.

@PascalHonegger
Copy link
Collaborator Author

After updating the test files, there seems to be some issues with the localforage code where it doesn't seem to work with the tests anymore. I'm not sure what broke or how to fix it, but honestly the whole test setup is probably one of the things that needs to be completely redone eventually.

Yeah I think the modern approach would be to switch the whole build to vite and the tests to vitest, atleast that's what I believe to be the easiest and fastest tooling available at the time. I could try to do that next, but I would first have to understand what the desired state is (which seems quite complex at the moment, but I think just ReScript compile + bundle is all that would be needed?)

Should I continue on this PR or is this "mergable" even though some tests and the "Externals/Numeral.rs" is still outstanding?

@PascalHonegger PascalHonegger marked this pull request as ready for review March 28, 2025 21:09
@johnridesabike
Copy link
Owner

After thinking about it, I decided to simply remove the Numeral.Helpers module since it wasn't being used anyway. I had originally intended for the Numeral module to cover the entire Numeral JS API, but that was never really needed and now I don't think it's worth the maintenance.

I'm happy to merge this and address the test suite and build system in a separate PR. Since the live site is built from the master branch, I'm first going to double-check that there aren't any obvious quirks in the build before merging though.

Copy link
Owner

@johnridesabike johnridesabike left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you for working on this!

@johnridesabike johnridesabike merged commit 01caa1a into johnridesabike:master Mar 30, 2025
4 checks passed
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.

2 participants