🦋 Changeset detected

Latest commit: e7fa7e2

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Hi @silvenon,

Welcome, and thank you for contributing to React Router!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

i think we should add a test here @silvenon

if rr team had a test for this config in the first place, it would've been caught before shipping to production (i think, not sure if this is on vite or rr team here to be fair 😅 ), im not entirely sure what the conventions here are etc.

i was playing around with code quickly, but the test case below should've caught the issue

i imagine it looking like:

import { describe, it, expect, beforeEach } from 'vitest';

type DevProdCondition = 'development' | 'production';
type BaseCondition = 'module' | DevProdCondition;
type ServerCondition = BaseCondition | 'node' | 'workerd' | 'worker';

interface ViteConfig {
 ssr?: {
   resolve?: {
     conditions?: string[];
   }
 }
}

interface ConditionsConfig {
 vite: {
   defaultServerConditions?: string[];
 };
 userConfig?: ViteConfig;
 command: 'build' | 'serve';
}

function getServerConditions({
 vite,
 userConfig,
 command
}: ConditionsConfig): string[] {
 // Check user config first
 if (userConfig?.ssr?.resolve?.conditions !== undefined) {
   return userConfig.ssr.resolve.conditions;
 }

 // Get Vite defaults or empty array if old Vite version
 const defaultConditions = "defaultServerConditions" in vite
   ? Array.from(vite.defaultServerConditions)
   : [];

 // Add development in dev mode
 return command === 'build' 
   ? defaultConditions
   : ['development', ...defaultConditions];
}

describe('Server Conditions', () => {
 let mockVite: ConditionsConfig['vite'];
 let mockUserConfig: ViteConfig;
 
 beforeEach(() => {
   // Reset mocks before each test
   mockVite = {
     defaultServerConditions: ['module', 'node']
   };
   mockUserConfig = {};
 });

// a case like this would've caught the issue ensuring conditions isn't empty
 it('should use Vite default server conditions in build mode', () => {
   const conditions = getServerConditions({
     vite: mockVite,
     userConfig: mockUserConfig,
     command: 'build'
   });
   
   expect(conditions).toEqual(['module', 'node']);
 });
});

PS.

im just a community contributor

rr team are the ones calling the shots ❤️ 🫡

i think we should add a test here

I'd like to add a test as well, but I'd like to wait for some team feedback first, I don't really know how to test this, but thanks for your suggestion.

TBH I don't know why the "development" condition is even being forced at all. I thought that Vite by default resolves to "development" condition during development mode and to "production" condition during build both in Vite v5 and v6.

@markdalgleish done. Do you know what the reason is for adding the "development" condition in the first place? Is it for virtual modules?

@silvenon It was added in this PR: #12269 — but this change was later rolled back. Looks like the condition was accidentally left in, but as you pointed out, it might not have been needed in the first place. We could look at removing this in a separate PR.

I don't like rebasing, but if there's one circumstance where it's appropriate, it's when I literally based my branch off the wrong one. Here, everything clean now.

You're welcome, I'm very glad this got merged, thanks for your reviews!