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

fix: semver comparison #216

Merged
merged 3 commits into from
Dec 13, 2023
Merged

Conversation

Cherry
Copy link
Contributor

@Cherry Cherry commented Dec 9, 2023

fixes #215
closes #214

This also bumps a few dependencies, to remove some npm audit warnings. The output bundle increases from 964kB to 983kB, but I feel the 19kB increase is worth the reliable semver comparison.

chore: bump dependencies
@Cherry Cherry requested a review from a team as a code owner December 9, 2023 18:29
@Cherry Cherry force-pushed the fix/semver-check branch 2 times, most recently from 2dbf0c7 to c559f04 Compare December 9, 2023 18:46
@Cherry
Copy link
Contributor Author

Cherry commented Dec 11, 2023

Not sure what's up with the test failure here, that's not one that I touched 🤔

- "Directory /does/not/exist does not exist."
+ [Error: Directory /does/not/exist does not exist.]

Seems like the test is doing what it expected, but not checking for the right error type?

@petebacondarwin
Copy link
Contributor

The failed test is due to updating vitest to v1, which contains this breaking change: vitest-dev/vitest#4396.

The resolution is just to fix up the snapshot to match the new expected value.

@Cherry
Copy link
Contributor Author

Cherry commented Dec 12, 2023

Great catch, thanks @petebacondarwin. I've updated that now and all should be good.

@petebacondarwin
Copy link
Contributor

Hmm. The failing tests need to be run on our fork since they require secrets...

@Cherry
Copy link
Contributor Author

Cherry commented Dec 12, 2023

Yeah I can't really fix that one unfortunately 😞 I've not made any real changes to the deploy process of this, but let me know if I can assist with anything further here to get this merged.

@petebacondarwin
Copy link
Contributor

Well that is interesting! By pushing a branch with the exact commit from this PR in it, the CI tests ran and Github attributed the successful test run to this PR too! TIL!

All good to go.

Thanks @Cherry

@petebacondarwin petebacondarwin merged commit 9aba9c3 into cloudflare:main Dec 13, 2023
4 of 5 checks passed
@github-actions github-actions bot mentioned this pull request Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

action doesn't use bulk secret upload when specifying wranglerVersion 3.10.0 or higher
3 participants