Skip to content

fix(sveltekit): add Vite peer dep for proper type resolution #12926

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

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Jul 15, 2024

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Hello! I am in the process of setting up the SvelteKit plugin within a monorepo that includes multiple applications with multiple versions of Vite. I've found that because @sentry/sveltekit does not specify a peer dependency on vite, pnpm picks up the wrong vite version for the package's import type { Plugin } from 'vite' statement, causing type errors in my Vite config.

This PR adds a simple, optional peer dependency on Vite to @sentry/sveltekit to ensure that it picks up whatever version of Vite is being used in the specific package it's installed in, without auto-installing vite or anything disruptive like that. Let me know if you'd prefer specific versions spec'd rather than *, or if you'd prefer to not mark the dependency as optional: true

Note

I set up Volta as instructed in the Contributing guide and used the spec'd versions of Node and yarn. None of yarn build, yarn lint, nor yarn test fully passed on my machine on a fresh copy of the develop branch

@Lms24 Lms24 self-requested a review July 16, 2024 08:00
@Lms24 Lms24 self-assigned this Jul 16, 2024
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Hey @mcous thanks a lot for opening this PR!

I had no idea that pnpm would pick the wrong version. I guess that's another entry in the book of pnpm's weird module resolution behaviours 😅

The change looks fine to me. In our docs, we recommend users to use Vite 4.2 or newer. However, we phrase it in a way that it's not strictly required. So for now I vote we continue with * as a version to fix.

I just started CI. Once everything is green, I'll merge this PR.

(heads-up: Just assigning myself to this PR to track internally that I'm reviewing it)

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Tests (including SvelteKit e2e tests) passed, so let's merge this. Thanks for contributing @mcous!

@Lms24 Lms24 merged commit 475d66f into getsentry:develop Jul 16, 2024
85 checks 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.

2 participants