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: vite build binding not found error #90

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

yann510
Copy link
Collaborator

@yann510 yann510 commented Oct 31, 2023

closes #89

Copy link
Owner

@shannonhochkins shannonhochkins left a comment

Choose a reason for hiding this comment

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

Also a bit concerned why your lock file is so different after installing a few packages

@@ -24,7 +24,7 @@
},
"engines": {
"node": ">=16.0.0",
"npm": "^7.0.0"
"npm": ">=7.0.0"
Copy link
Owner

Choose a reason for hiding this comment

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

Why did you change this? Any particular reason?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm using node 18 with npm 9 and this was giving me warnings.
For the lock file, maybe its because I'm using npm 9? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should probably build and run the tests in the CI to ensure pull requests are working at a minimum level

Copy link
Owner

Choose a reason for hiding this comment

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

Tests have been broken for a while, I decided to park tests while the architecture was changing so much 🙃 all good I'd say the biggest descrepency here with the lock file, is that you most likely followed step 1 in the link I sent? If you had removed the lock file then run npm I it explains the diff here :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I added some new dev packages to ensure it works properly for everyone.
I don't think a big change in the lock file is an issue, it is actually expected when you add new packages :)

I've been testing with linked package on my test dashboard and everything seems to be working just fine.

Need anything changed before we merge this?

Copy link
Owner

Choose a reason for hiding this comment

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

Sweet, and nope no changes, just need some time to check it out and test it on my machine to ensure it works for me 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Awesome, keep me posted :)

@shannonhochkins shannonhochkins merged commit aedf77c into shannonhochkins:master Nov 3, 2023
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.

Getting a lot of errors when following the contribution documentation
2 participants