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

Update all dependencies (incl JS) and make use of thread_local #12

Merged
merged 14 commits into from
Oct 3, 2024

Conversation

9SMTM6
Copy link
Contributor

@9SMTM6 9SMTM6 commented Sep 13, 2024

Builds on the work of @th4s.

Also bumps rust editions, with a new edition coming in soon I think thats good to have more up to date. the projects also got updates to their JS dependencies.

wasm-pack is still out of date, stuck on #9.

I wasn't able to run the exact CI sequence on my laptop since I dont run ubuntu, and playwright install doesnt seem to like that, but with a bit of nudging it seemed to work.

closes #10

.github/workflows/ci.yml Outdated Show resolved Hide resolved
test/rust-toolchain.toml Outdated Show resolved Hide resolved
@9SMTM6
Copy link
Contributor Author

9SMTM6 commented Oct 3, 2024

@RReverser not sure why that test failed. It ran fine on my fork.

I think it was a bit flaky for me when I ran it locally, before some other changes, I thought these fixed it. Were they ever flaky in the past?

@9SMTM6
Copy link
Contributor Author

9SMTM6 commented Oct 3, 2024

@RReverser Ive now fixed the more recent issues in the CI, not super elegant, but IDK another way to trigger the installation according to toolchain.toml.

The issues we saw in the first CI run in this repo (2 days ago, commit d1816e5) have not really been addressed specifically, so they might reappear.

I removed --with-deps from playwright install, seems to work regardless - I guess githubs VMs already have all required dependencies -, and this solves some of the issues I encountered. Should probably also solve the CI failure in #13.

@9SMTM6
Copy link
Contributor Author

9SMTM6 commented Oct 3, 2024

I've been also so free to remove the CLA reference I mentioned in #10.

@RReverser
Copy link
Owner

Weird. I don't think it was flaky in the past, no, but entirely plausible something on their server has changed.

Thanks!

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@RReverser
Copy link
Owner

Awesome, thanks @9SMTM6 and @th4s both!

@RReverser
Copy link
Owner

warning: unused return value of clone that must be used
--> src/lib.rs:58:5
|
58 | wasm_bindgen::link_to!(module = "/src/workerHelpers.worker.js");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|

Can you suppress this please? I imagine all users will get this warning when building this as a transitive dependency, and it's just noise at this point.

@RReverser RReverser merged commit 8bc4577 into RReverser:main Oct 3, 2024
1 check 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.

3 participants