Skip to content

Conversation

@onlywei
Copy link
Collaborator

@onlywei onlywei commented May 1, 2025

npm always tries to install transitive optionalDependencies by default, even if you don't need or use them.

This means that every time someone does npm i @fastify/vite, they will also result install these other packages into their node_modules directory, unless they explicitly run npm i @fastify/vite --omit=optional.

This is not only wasteful of our users' HDD space, but it also can cause weird issues with tools like snyk. For example, when I installed @fastify/vite, it came with @fastify/htmx, but it did not come with the dependencies of @fastify/htmx. Specifically, the @kitajs/html was not installed. snyk didn't know what to do in this situation -- it saw @kitajs/html in package-lock.json but didn't find it in node_modules, so it thought my package-lock.json was out of sync with my package.json and threw an error.

Is there any advantage we gain from adding these things in optionalDependencies? If not, I think it's best to just delete them.

Checklist

@onlywei onlywei requested a review from galvez May 1, 2025 21:49
@netlify
Copy link

netlify bot commented May 1, 2025

Deploy Preview for agitated-mahavira-26f8f9 canceled.

Name Link
🔨 Latest commit 441a77e
🔍 Latest deploy log https://app.netlify.com/sites/agitated-mahavira-26f8f9/deploys/681561f67ab06e000813449a

@galvez
Copy link
Member

galvez commented May 1, 2025

Can you update package.mjs to include/remove optionalDependencies — they're required for dev.

@onlywei
Copy link
Collaborator Author

onlywei commented May 2, 2025

Can you update package.mjs to include/remove optionalDependencies — they're required for dev.

I don't see any mention of optionalDependencies in package.mjs

@galvez
Copy link
Member

galvez commented May 2, 2025

There isn't, since they were part of @fastify/vite's package.json — the setup worked for both releases and development. Since there's a problem with keeping them on the release, the prep-for-dev script should add them to @fastify/vite's package.json, and the prep-for-release script should remove them.

@onlywei
Copy link
Collaborator Author

onlywei commented May 2, 2025

There isn't, since they were part of @fastify/vite's package.json — the setup worked for both releases and development. Since there's a problem with keeping them on the release, the prep-for-dev script should add them to @fastify/vite's package.json, and the prep-for-release script should remove them.

What's the reason they're needed for development?

@galvez
Copy link
Member

galvez commented May 2, 2025

When developing the starters, somehow, local @fastify/react and @fastify/vue will fail to be picked up unless they're listed in @fastify/vite.

@zanmato
Copy link
Collaborator

zanmato commented May 2, 2025

It's because the renderer is being passed by string to fastify/vite which can't resolve it. If we pass it like this it'll work, and we can remove the optional dependencies,

await server.register(FastifyVite, {
  root: import.meta.url,
  renderer: import.meta.resolve('@fastify/vue'),
})

@zanmato
Copy link
Collaborator

zanmato commented May 2, 2025

We should update the ci file to also run test on main I guess, this PR bypassed them

onlywei added 2 commits May 2, 2025 16:35
npm always tries to install optionalDependencies by default, even
if you don't need or use them. This means that every @fastify/vite
installation will also result in all these other packages entering
node_modules.
@onlywei onlywei force-pushed the stop-optional-dependencies branch from a66515d to 5b1a089 Compare May 2, 2025 20:35
@onlywei onlywei changed the base branch from main to dev May 2, 2025 20:35
@onlywei onlywei requested a review from zanmato May 2, 2025 20:35
@onlywei
Copy link
Collaborator Author

onlywei commented May 2, 2025

Alrighty. @zanmato @galvez I switched to import.meta.resolve as suggested.

@galvez
Copy link
Member

galvez commented May 2, 2025

No, please, that's unnecessary complexity. I'm okay with using import.meta.resolve() in development mode, but it certainly shouldn't be seen in the docs. Mind if I update directly in this branch @onlywei?

@galvez galvez marked this pull request as draft May 2, 2025 20:49
@galvez galvez self-assigned this May 3, 2025
@galvez galvez marked this pull request as ready for review May 3, 2025 00:23
@galvez galvez merged commit 1e96409 into dev May 3, 2025
4 of 5 checks passed
@galvez galvez changed the title fix: Remove optionalDependencies from @fastify/vite fix: remove optionalDependencies from @fastify/vite May 3, 2025
@onlywei onlywei deleted the stop-optional-dependencies branch May 3, 2025 20:21
onlywei added a commit that referenced this pull request May 3, 2025
* fix: Remove optionalDependencies from @fastify/vite (#257)

* docs: rm outdated info

* docs: update roadmap

* docs: update known limitations

* docs: update section on route location

* chore: fix pnpm-lock.yaml

* Import meta dirname filename (#263)

* Use import.meta.dirname instead of import.meta.url

* Use import.meta.filename

* undo changes in contrib folder

* Fix some stuff I missed

* Add src/ directory to TypeScript starters (#264)

* Bump @fastify/[email protected]

---------

Co-authored-by: Jonas Galvez <[email protected]>
onlywei added a commit that referenced this pull request Aug 4, 2025
* fix: Remove optionalDependencies from @fastify/vite (#257)

* docs: rm outdated info

* docs: update roadmap

* docs: update known limitations

* docs: update section on route location

* chore: fix pnpm-lock.yaml

* Import meta dirname filename (#263)

* Use import.meta.dirname instead of import.meta.url

* Use import.meta.filename

* undo changes in contrib folder

* Fix some stuff I missed

* Add src/ directory to TypeScript starters (#264)

* Bump @fastify/[email protected]

* fix: install tsx into react/vue starters for typescript

* fix: set allowJs to false in starters' tsconfig.json files

* Bump @fastify/[email protected]

* Run pnpm prep-for-release

---------

Co-authored-by: Jonas Galvez <[email protected]>
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.

4 participants