-
Notifications
You must be signed in to change notification settings - Fork 46
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
changes to devDependencies that seem necessary for main to work #826
Conversation
https://viv-avivator-hq3v-git-avivator-vercel-xinaesthetes-projects.vercel.app/ LGTM. Pretty sure main in its current state fails without these changes. I could really do with getting the other PR merged soon as I have deadlines and it's a bit of a blocker. Cheers, |
I notice there's also some other CI failing on main with some fairly trivial biome stuff;
Also it doesn't show up in CI logs but I noticed a warning from biome that |
I just made #827, which should resolve the biome stuff going forward. Sorry about that. You should be able to merge in main and revert the fixes for the latest version of biome. |
9e78242
to
b605672
Compare
@manzt I did a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for opening a PR for discussion. I apologize if I'm missing some context, but it would be really useful to look at the logs from Vercel. I assume we're using some integration with Vercel on your fork to get previews of the various PRs you're opening.
While that's great, I also want to understand why we need to make these changes. It would be more useful to investigate your Vercel setup rather than make changes to Viv, especially since this isn't something we need to host.
I'm not an avid user of Vercel, but I understand they are very oriented around NextJS apps. Their "out of the box" setup tries to do some magic with installations and build steps. The purpose of devDependencies is to separate what's required in the website from what's required to build the website. This is useful for maintainers to understand the relationships between dependencies.
With Vercel, my understanding is that they assume you're making a NextJS app, and therefore they will "skip" devDependencies since they know what's typically required to build NextJS apps. Their default behavior is generally to only install production dependencies.
Have you specified a custom install command in your Vercel setup? I think pnpm install
should do the trick. My guess is the default behavior by Vercel is to exclude development dependencies since they know the toolchain for NextJS apps.
@@ -34,7 +34,7 @@ | |||
"@svitejs/changesets-changelog-github-compact": "^1.1.0", | |||
"esbuild": "^0.19.5", | |||
"esno": "^4.0.0", | |||
"gl": "^6.0.2", | |||
"gl": "^8.0.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this causing an issue in Vercel or for you locally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
locally - couldn't pnpm i
without this
To be honest I think I set the Vercel thing up ages ago and then I wasn't quite clear myself whether it was something I'd done or not. So no - as of now I hadn't setup a custom install command etc as I somewhat assumed this was more generally relevant to you than it actually was. |
No worries about the uncertainty with the Vercel setup. I'm fairly convinced this issue can be fixed with some configuration since we build and deploy Avivator in various platforms (GitHub CI, Netflify) and would prefer not to make changes that change the semantics and readability of our code base. |
Let me have a look on Vercel now. I'll send a config that works (if I can get it going). |
Absolutely - I have no desire to interfere, it just seemed like there was something preventing So it does seem that the The |
Yah I definitely thing gl is an issue (classic node-gyp!). I'll have a look. I'm guessing we are on a newer version of node now and that's causing an issue.
Agreed, good catch. |
Ok I got I think I'm not sure what system vercel builds on, but the binaries for The following {
"buildCommand": "pnpm --filter=avivator build",
"installCommand": "pnpm remove gl && pnpm build",
"outputDirectory": "sites/avivator/dist"
} or in the UI under your project settings: Also make sure the Root Directory is blank. Sorry for the workaround, but I'll work on a PR to fix up |
Should be able to remove |
So I think this PR is probably redundant now and could be closed? |
Yup! Let me know if you have trouble building avivator. |
As of this writing, this is in a draft state - I want to verify that Vercel does indeed fail when changes to avivator dependencies aren't made.
Fixes #825