Simplify Vite Plugin

[evoactivity]

This will allow the vite config to just be

import { defineConfig } from 'vite';
import ember from '@embroider/vite';

export default defineConfig({
  plugins: [ember()],
});

[NullVoxPopuli]

I liked your earlier suggestion of having babel present in the userland vite config.

folks may want to change this and it isn't "our" plugin

[evoactivity]

babel would be configured with the babel config file, not in the vite config.

See react and vue vite plugins, they also setup babel but it's ultimately configured by the user with a babel config file.
https://github.com/vitejs/vite-plugin-react/blob/main/packages/plugin-react/src/index.ts
https://github.com/vitejs/vite-plugin-vue/blob/599f81369816aca918314e86f5b0a6e32367bb38/packages/plugin-vue-jsx/src/index.ts#L4

[NullVoxPopuli]

I guess if we're confident in our defaults, and want to align with everyone else, I'm game.

[evoactivity]

I'll leave this one open to see if anyone else has an opinion. I would vote for keeping this internal though, as users should only need to worry about babel.config.js, not how it actually is run on the files.

[NullVoxPopuli]

I think build, and the choice to configure tests should be left to the user.

right now there some unease around /tests/ and the escape hatch we have in the existing vite config is to encourage changing /tests/ to /tests__whatever_i_want/

[evoactivity]

See #2183 (comment)

[NullVoxPopuli]

for this one, specifically, I think it's better to expose always, for discoverability

[evoactivity]

what if you could just do and we have this in the userland vite config?

ember({
  testUrl: 'tests/'
})

[NullVoxPopuli]

why invent and maintain a new API tho?

[NullVoxPopuli]

(i'll defer to others on this if they disagree with me, ofc)

[evoactivity]

Actually maybe I need to understand this unease and escape hatch a litte more.

How would the user configure this, they would also need to rename the tests folder wouldn't they?

[NullVoxPopuli]

excited for shrinking the "default" config, but i left some comments/questions/concerns

[NullVoxPopuli]

ope

[evoactivity]

Anyone have an idea why the CI bails out saying the lockfile needs to be updated? (I've ran pnpm and lockfile is not changing between runs)

[NullVoxPopuli]

does your tooling change your pnpm version based on the packageManager field? odds are you're using newer pnpm, and this repo is on pnpm 8

[evoactivity]

I'm using volta with the pnpm flag enabled, so I think it should be, but it's installing 9 instead of 8, so I guess it's not.

[NullVoxPopuli]

yeah, we don't have a volta config in the package.jsons

[evoactivity]

Ah ok, this would suggest otherwise
https://github.com/embroider-build/embroider/blob/main/CONTRIBUTING.md

[evoactivity]

Downgraded to pnpm 8.15.8 using corepack and now getting a strange typescript issue when running pnpm install, showing in CI as well.

prepare$ tsc -b && pnpm build-v2-addons
│ packages/macros/tests/babel/helpers.ts(85,15): error TS2454: Variable 'mode' is used before being assigned.

but it's not used before being assigned

embroider/packages/macros/tests/babel/helpers.ts

Lines 82 to 85 in 0aa8fc6

	for (let mode of ['build-time', 'run-time']) {
	describe(mode, function () {
	function applyMode(macrosConfig: MacrosConfig) {
	if (mode === 'run-time') {

Changing it to a const for (const mode of ['build-time', 'run-time']) { makes it work 🫤

[evoactivity]

Looks like typescript updated. Should that be pinned instead since stuff breaks often with typescript minor updates?

I don't understand why let mode is broken and const mode is fine, but 🤷