Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughLos cambios incluyen actualizaciones de gestión de dependencias en el ecosistema ES-JS, migración hacia referencias de espacios de trabajo, adición de configuración de resolución de rutas, incrementos de versión de paquetes, exportación pública de EsCSS desde el complemento vite, e implementación de infraestructura de pruebas integral con accesorios y suites de pruebas. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutos Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/vite-plugin-escss/package.json (1)
24-33:⚠️ Potential issue | 🟠 Major
@es-js/escssdebería estar endependencies, no endevDependencies.El plugin importa
compilede@es-js/escssen runtime (src/index.ts, línea 1). Al tenerlo solo endevDependencies, los consumidores que instalen@es-js/vite-plugin-escssno obtendrán@es-js/escsstransitivamente, provocando un error en runtime.Adicionalmente,
vitesuele declararse comopeerDependencyen plugins de Vite, no comodependencydirecta, para evitar duplicar la instancia de Vite.Corrección propuesta
"dependencies": { - "vite": "^5.0.0" + "@es-js/escss": "workspace:*" + }, + "peerDependencies": { + "vite": "^5.0.0" }, "devDependencies": { - "@es-js/escss": "workspace:*", "@types/node": "^20.14.9",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vite-plugin-escss/package.json` around lines 24 - 33, Mueve "@es-js/escss" de devDependencies a dependencies en package.json porque se importa en tiempo de ejecución (ver import compile en src/index.ts); además cambia "vite" a peerDependencies (y opcionalmente déjalo en devDependencies solo para el entorno de desarrollo) para evitar duplicar Vite en el consumidor; actualiza la sección dependencies/devDependencies/peerDependencies en package.json en consecuencia y ajusta cualquier build/test workflow que asuma la ubicación anterior.packages/vite-plugin-escss/src/index.ts (2)
157-158: 🧹 Nitpick | 🔵 TrivialLa línea
export { EsCSS }re-exporta el default export como named export — correcto pero redundante con elexport default.Los consumidores ya pueden hacer
import EsCSS from '...'oimport { default as EsCSS } from '...'. Si la intención es permitirimport { EsCSS } from '...', está bien, pero documéntelo explícitamente para evitar confusión.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vite-plugin-escss/src/index.ts` around lines 157 - 158, The file re-exports the default as a named export with "export { EsCSS }", which is redundant if you only intend a default export; either remove the named re-export to avoid confusion or keep it but add an explicit comment and update docs explaining that consumers may import as either default or named. Locate the symbol EsCSS in this module (packages/vite-plugin-escss/src/index.ts) and either delete the "export { EsCSS }" line, or retain it and add a clear inline comment and README note clarifying that "import { EsCSS } from '...'" is intentionally supported.
86-104:⚠️ Potential issue | 🔴 CriticalVulnerabilidad de path traversal en el middleware del servidor de desarrollo.
Un atacante puede enviar una petición como
GET /../../etc/passwd.escssy, si el archivo existiese con extensión.escss, leer su contenido.path.resolve(root, cleanUrl.slice(1))no valida que la ruta resultante esté dentro deroot.Corrección propuesta
const cleanUrl = url.split('?')[0] const filePath = path.resolve(root, cleanUrl.slice(1)) + // Prevent path traversal outside project root + if (!filePath.startsWith(root + path.sep) && filePath !== root) return next() + if (!existsSync(filePath)) return next()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vite-plugin-escss/src/index.ts` around lines 86 - 104, La ruta resuelta con path.resolve(root, cleanUrl.slice(1)) en el middleware server.middlewares.use puede escapar de root; antes de leer con existsSync/readFileSync revise que la ruta normalizada esté dentro de root (por ejemplo usando path.relative(root, filePath) o comparando que filePath.startsWith(path.resolve(root) + path.sep)) y si no lo está devuelva next() o un 403 sin leer el archivo; aplique esta validación entre la obtención de filePath y la comprobación existsSync para proteger compile(...) y readFileSync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demos/demo-escss/vite.config.js`:
- Around line 25-34: Falta la extensión '.escss' en la lista resolve.extensions
dentro de vite.config.js: en la propiedad resolve.extensions (el arreglo que
actualmente contiene '.esjs' y '.eshtml') añade '.escss' para que Vite pueda
resolver importaciones sin extensión; actualiza el array resolve.extensions (en
el archivo que define esa variable) y confirma el orden consistente con las
demás extensiones.
In `@packages/escss/package.json`:
- Around line 24-26: Actualizar el lockfile para que incluya la nueva
dependencia movida: ejecutar localmente `pnpm install` en el paquete (donde
package.json define "postcss" en dependencies) para regenerar pnpm-lock.yaml,
verificar que la dependencia `postcss` aparece en el lockfile y commitear el
lockfile actualizado junto con el cambio de package.json; esto asegura que la
función `compile` que usa postcss en runtime tenga la dependencia resuelta y que
`pnpm install --frozen-lockfile` pase en CI.
In `@packages/vite-plugin-escss/src/index.ts`:
- Around line 59-65: The resolveId function can be tricked by path traversal in
sourceId; after computing fullPath (using path.resolve(base, sourceId)) validate
that it stays inside root by comparing path.relative(root, fullPath) and
returning null if it starts with '..' or is absolute outside root; use
path.normalize or path.resolve to canonicalize both root and fullPath before the
check, then continue the existing existsSync and replacement logic (references:
resolveId, sourceId, importer, root, existsSync, ESCSS_QUERY).
- Around line 43-44: El array compartido escssAssetsToEmit se declara fuera del
ciclo de build y solo se limpia en buildStart, lo que permite acumulación si
transformIndexHtml se llama sin buildStart (p.ej. en dev/HMR); para arreglar,
asegúrate de reinicializar o limpiar escssAssetsToEmit al inicio de buildStart y
también al inicio de transformIndexHtml y/o writeBundle, y al hacer push hacia
escssAssetsToEmit deduplica comprobando fileName (o usando un Set de nombres)
para evitar entradas duplicadas; referencia las funciones/constantes
escssAssetsToEmit, buildStart, transformIndexHtml, writeBundle y ESCSS_QUERY al
aplicar estos cambios.
- Around line 130-132: El problema es que usar path.basename(cleanHref,
'.escss') para generar baseName provoca colisiones (p. ej. pages/estilos.escss y
components/estilos.escss) y un sobrescrito silencioso en escssAssetsToEmit y
writeBundle; para arreglarlo, derive fileName de una clave única en vez de solo
el basename: por ejemplo incluir parte de la ruta (normalizada) o un hash corto
del contenido `compiled` al construir baseName/fileName, actualiza la lógica que
añade a escssAssetsToEmit ({ fileName, source: compiled }) y ajusta writeBundle
para usar esa misma clave única; modifica los sitios donde se usa
baseName/fileName (las variables baseName, fileName, la llamada a
path.basename(cleanHref, '.escss'), escssAssetsToEmit y la función writeBundle)
para garantizar nombres únicos y consistentes entre generación y escritura.
- Around line 144-153: The writeBundle hook signature is incorrect; change the
function signature from writeBundle(outputOptions: { dir?: string }) to the
proper Rollup/Vite types (e.g., writeBundle(options: NormalizedOutputOptions,
bundle: OutputBundle)) by importing NormalizedOutputOptions and OutputBundle
from 'rollup' (or vite types), then use options.dir in the body; keep the
existing logic that checks enabled and escssAssetsToEmit and emitting files, but
update the parameter name and types to restore proper type-checking for
writeBundle, escssAssetsToEmit, and enabled.
In `@packages/vite-plugin-escss/test/vite-plugin-escss.test.ts`:
- Around line 65-86: The test for transformIndexHtml currently checks HTML
output but doesn't assert the plugin recorded the compiled asset; update the
test to also assert that the plugin's escssAssetsToEmit (the collection
populated by transformIndexHtml) contains an entry for 'estilos.escss' mapping
to the expected output filename (e.g., 'assets/estilos.css') so writeBundle will
emit it; locate where handler (from transformIndexHtml) is used and add an
assertion on plugin.escssAssetsToEmit (or the exact property name used in the
plugin) after calling handler to ensure the asset was registered for emission by
writeBundle.
- Around line 55-63: The test repeatedly casts result to { code: string };
assign the typed value once to a local variable (e.g., const loaded = result as
{ code: string }) after calling plugin.load!(resolvedId) and then use
loaded.code in the three assertions to improve readability; update the test
around the plugin.load!/resolvedId usage accordingly (retain same assertions but
replace repeated casts with the single typed variable).
- Around line 9-18: Add unit tests covering the missing plugin hooks:
writeBundle, configureServer and buildStart. Create the plugin instance via
EsCSS() (as in the existing beforeAll), call plugin.configResolved({ root:
fixturesDir }) then invoke plugin.buildStart() and assert it clears the internal
assets array (or resets expected state); for writeBundle create a temporary
directory with fs.mkdtempSync, call plugin.writeBundle({} as any, { dir: tempDir
} as any) or the hook signature used in the plugin and verify expected files are
written to disk; for configureServer create a mock ViteDevServer object (with
use/middlewares or ws stubs as needed), call plugin.configureServer(server) and
assert the middleware or websocket handlers were registered. Ensure tests
reference EsCSS, plugin.buildStart, plugin.writeBundle and
plugin.configureServer so they exercise those hooks and clean up the temp
directory after assertions.
---
Outside diff comments:
In `@packages/vite-plugin-escss/package.json`:
- Around line 24-33: Mueve "@es-js/escss" de devDependencies a dependencies en
package.json porque se importa en tiempo de ejecución (ver import compile en
src/index.ts); además cambia "vite" a peerDependencies (y opcionalmente déjalo
en devDependencies solo para el entorno de desarrollo) para evitar duplicar Vite
en el consumidor; actualiza la sección
dependencies/devDependencies/peerDependencies en package.json en consecuencia y
ajusta cualquier build/test workflow que asuma la ubicación anterior.
In `@packages/vite-plugin-escss/src/index.ts`:
- Around line 157-158: The file re-exports the default as a named export with
"export { EsCSS }", which is redundant if you only intend a default export;
either remove the named re-export to avoid confusion or keep it but add an
explicit comment and update docs explaining that consumers may import as either
default or named. Locate the symbol EsCSS in this module
(packages/vite-plugin-escss/src/index.ts) and either delete the "export { EsCSS
}" line, or retain it and add a clear inline comment and README note clarifying
that "import { EsCSS } from '...'" is intentionally supported.
- Around line 86-104: La ruta resuelta con path.resolve(root, cleanUrl.slice(1))
en el middleware server.middlewares.use puede escapar de root; antes de leer con
existsSync/readFileSync revise que la ruta normalizada esté dentro de root (por
ejemplo usando path.relative(root, filePath) o comparando que
filePath.startsWith(path.resolve(root) + path.sep)) y si no lo está devuelva
next() o un 403 sin leer el archivo; aplique esta validación entre la obtención
de filePath y la comprobación existsSync para proteger compile(...) y
readFileSync.
| extensions: [ | ||
| '.js', | ||
| '.json', | ||
| '.jsx', | ||
| '.mjs', | ||
| '.ts', | ||
| '.tsx', | ||
| '.esjs', | ||
| '.eshtml', | ||
| ], |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Falta .escss en resolve.extensions.
Se incluyeron .esjs y .eshtml pero no .escss. Si algún archivo importa un .escss sin extensión explícita, Vite no lo resolverá. Si los .escss solo se referencian con extensión completa o desde <link>, no es un problema, pero por consistencia considere añadirlo.
Sugerencia
'.esjs',
'.eshtml',
+ '.escss',
],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@demos/demo-escss/vite.config.js` around lines 25 - 34, Falta la extensión
'.escss' en la lista resolve.extensions dentro de vite.config.js: en la
propiedad resolve.extensions (el arreglo que actualmente contiene '.esjs' y
'.eshtml') añade '.escss' para que Vite pueda resolver importaciones sin
extensión; actualiza el array resolve.extensions (en el archivo que define esa
variable) y confirma el orden consistente con las demás extensiones.
| "dependencies": { | ||
| "postcss": "^8.4.38" | ||
| }, |
There was a problem hiding this comment.
El lockfile no está actualizado — el pipeline CI falla.
El pipeline de CI reporta que pnpm install --frozen-lockfile falla porque el lockfile no refleja la nueva dependencia postcss en dependencies. Es necesario ejecutar pnpm install para regenerar el lockfile y commitear los cambios.
Mover postcss de devDependencies a dependencies es correcto dado que se usa en runtime por la función compile.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/escss/package.json` around lines 24 - 26, Actualizar el lockfile
para que incluya la nueva dependencia movida: ejecutar localmente `pnpm install`
en el paquete (donde package.json define "postcss" en dependencies) para
regenerar pnpm-lock.yaml, verificar que la dependencia `postcss` aparece en el
lockfile y commitear el lockfile actualizado junto con el cambio de
package.json; esto asegura que la función `compile` que usa postcss en runtime
tenga la dependencia resuelta y que `pnpm install --frozen-lockfile` pase en CI.
| const ESCSS_QUERY = '?from=escss' | ||
| const escssAssetsToEmit: Array<{ fileName: string; source: string }> = [] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
escssAssetsToEmit se comparte entre builds pero su scope podría causar acumulación inesperada.
El array se declara en el closure de la función y se limpia en buildStart. Esto es correcto para builds normales, pero si transformIndexHtml se invocara sin un buildStart previo (e.g., en dev con HMR), los assets de invocaciones anteriores podrían acumularse. Considerar también limpiar al inicio de writeBundle o verificar duplicados al hacer push.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vite-plugin-escss/src/index.ts` around lines 43 - 44, El array
compartido escssAssetsToEmit se declara fuera del ciclo de build y solo se
limpia en buildStart, lo que permite acumulación si transformIndexHtml se llama
sin buildStart (p.ej. en dev/HMR); para arreglar, asegúrate de reinicializar o
limpiar escssAssetsToEmit al inicio de buildStart y también al inicio de
transformIndexHtml y/o writeBundle, y al hacer push hacia escssAssetsToEmit
deduplica comprobando fileName (o usando un Set de nombres) para evitar entradas
duplicadas; referencia las funciones/constantes escssAssetsToEmit, buildStart,
transformIndexHtml, writeBundle y ESCSS_QUERY al aplicar estos cambios.
| resolveId(sourceId: string, importer?: string) { | ||
| if (!enabled || !sourceId.endsWith('.escss')) return null | ||
| const base = importer ? path.dirname(importer) : root | ||
| const fullPath = path.resolve(base, sourceId) | ||
| if (!existsSync(fullPath)) return null | ||
| return fullPath.replace(/\.escss$/, '.css') + ESCSS_QUERY | ||
| }, |
There was a problem hiding this comment.
Misma vulnerabilidad de path traversal en resolveId.
path.resolve(base, sourceId) no verifica que la ruta resuelta quede dentro de root. Un sourceId malicioso con ../ podría resolver archivos fuera del proyecto. Aunque el riesgo es menor porque sourceId proviene del código fuente, es buena práctica defensiva validar el resultado.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vite-plugin-escss/src/index.ts` around lines 59 - 65, The resolveId
function can be tricked by path traversal in sourceId; after computing fullPath
(using path.resolve(base, sourceId)) validate that it stays inside root by
comparing path.relative(root, fullPath) and returning null if it starts with
'..' or is absolute outside root; use path.normalize or path.resolve to
canonicalize both root and fullPath before the check, then continue the existing
existsSync and replacement logic (references: resolveId, sourceId, importer,
root, existsSync, ESCSS_QUERY).
| const baseName = path.basename(cleanHref, '.escss') + '.css' | ||
| const fileName = `assets/${baseName}` | ||
| escssAssetsToEmit.push({ fileName, source: compiled }) |
There was a problem hiding this comment.
Colisión de nombres cuando varios .escss comparten el mismo basename.
path.basename(cleanHref, '.escss') descarta el directorio, así que pages/estilos.escss y components/estilos.escss generan ambos assets/estilos.css. El segundo sobrescribe al primero silenciosamente en escssAssetsToEmit y en writeBundle.
Considerar incluir un hash del contenido o preservar parte de la ruta para evitar colisiones:
Ejemplo de corrección
- const baseName = path.basename(cleanHref, '.escss') + '.css'
- const fileName = `assets/${baseName}`
+ const baseName = path.basename(cleanHref, '.escss')
+ // Use the relative path to avoid collisions
+ const safeName = cleanHref.replace(/\//g, '_').replace(/\.escss$/, '.css')
+ const fileName = `assets/${safeName}`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const baseName = path.basename(cleanHref, '.escss') + '.css' | |
| const fileName = `assets/${baseName}` | |
| escssAssetsToEmit.push({ fileName, source: compiled }) | |
| const baseName = path.basename(cleanHref, '.escss') | |
| // Use the relative path to avoid collisions | |
| const safeName = cleanHref.replace(/\//g, '_').replace(/\.escss$/, '.css') | |
| const fileName = `assets/${safeName}` | |
| escssAssetsToEmit.push({ fileName, source: compiled }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vite-plugin-escss/src/index.ts` around lines 130 - 132, El problema
es que usar path.basename(cleanHref, '.escss') para generar baseName provoca
colisiones (p. ej. pages/estilos.escss y components/estilos.escss) y un
sobrescrito silencioso en escssAssetsToEmit y writeBundle; para arreglarlo,
derive fileName de una clave única en vez de solo el basename: por ejemplo
incluir parte de la ruta (normalizada) o un hash corto del contenido `compiled`
al construir baseName/fileName, actualiza la lógica que añade a
escssAssetsToEmit ({ fileName, source: compiled }) y ajusta writeBundle para
usar esa misma clave única; modifica los sitios donde se usa baseName/fileName
(las variables baseName, fileName, la llamada a path.basename(cleanHref,
'.escss'), escssAssetsToEmit y la función writeBundle) para garantizar nombres
únicos y consistentes entre generación y escritura.
| writeBundle(outputOptions: { dir?: string }) { | ||
| if (!enabled || escssAssetsToEmit.length === 0) return | ||
| const outDir = outputOptions.dir | ||
| if (!outDir) return | ||
| for (const { fileName, source } of escssAssetsToEmit) { | ||
| const fullPath = path.join(outDir, fileName) | ||
| mkdirSync(path.dirname(fullPath), { recursive: true }) | ||
| writeFileSync(fullPath, source, 'utf-8') | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
La firma de writeBundle no coincide con la API de Vite/Rollup.
El hook writeBundle recibe (options: NormalizedOutputOptions, bundle: OutputBundle). La firma actual writeBundle(outputOptions: { dir?: string }) funciona en JS porque los argumentos extra se ignoran, pero dificulta la mantenibilidad y pierde el type-checking.
Corrección sugerida
- writeBundle(outputOptions: { dir?: string }) {
+ writeBundle(outputOptions) {O mejor aún, tipar correctamente importando los tipos de Rollup.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| writeBundle(outputOptions: { dir?: string }) { | |
| if (!enabled || escssAssetsToEmit.length === 0) return | |
| const outDir = outputOptions.dir | |
| if (!outDir) return | |
| for (const { fileName, source } of escssAssetsToEmit) { | |
| const fullPath = path.join(outDir, fileName) | |
| mkdirSync(path.dirname(fullPath), { recursive: true }) | |
| writeFileSync(fullPath, source, 'utf-8') | |
| } | |
| }, | |
| writeBundle(outputOptions, bundle) { | |
| if (!enabled || escssAssetsToEmit.length === 0) return | |
| const outDir = outputOptions.dir | |
| if (!outDir) return | |
| for (const { fileName, source } of escssAssetsToEmit) { | |
| const fullPath = path.join(outDir, fileName) | |
| mkdirSync(path.dirname(fullPath), { recursive: true }) | |
| writeFileSync(fullPath, source, 'utf-8') | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vite-plugin-escss/src/index.ts` around lines 144 - 153, The
writeBundle hook signature is incorrect; change the function signature from
writeBundle(outputOptions: { dir?: string }) to the proper Rollup/Vite types
(e.g., writeBundle(options: NormalizedOutputOptions, bundle: OutputBundle)) by
importing NormalizedOutputOptions and OutputBundle from 'rollup' (or vite
types), then use options.dir in the body; keep the existing logic that checks
enabled and escssAssetsToEmit and emitting files, but update the parameter name
and types to restore proper type-checking for writeBundle, escssAssetsToEmit,
and enabled.
| describe('vite-plugin-escss', () => { | ||
| let plugin: ReturnType<typeof EsCSS> | ||
|
|
||
| beforeAll(() => { | ||
| plugin = EsCSS() | ||
| if (Array.isArray(plugin)) { | ||
| throw new Error('Plugin returned array, expected single plugin') | ||
| } | ||
| plugin.configResolved?.({ root: fixturesDir } as any) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cobertura de tests incompleta: faltan writeBundle, configureServer y buildStart.
Los tests cubren bien los hooks principales (transform, resolveId, load, transformIndexHtml), pero no hay tests para:
writeBundle— escritura a disco de los assets compilados.configureServer— middleware del servidor de desarrollo.buildStart— limpieza del array de assets entre builds.
Considere añadir al menos un test para writeBundle usando un directorio temporal (fs.mkdtempSync) y verificando que los archivos se escriben correctamente.
¿Quiere que genere los tests faltantes o abra un issue para trackear esta tarea?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vite-plugin-escss/test/vite-plugin-escss.test.ts` around lines 9 -
18, Add unit tests covering the missing plugin hooks: writeBundle,
configureServer and buildStart. Create the plugin instance via EsCSS() (as in
the existing beforeAll), call plugin.configResolved({ root: fixturesDir }) then
invoke plugin.buildStart() and assert it clears the internal assets array (or
resets expected state); for writeBundle create a temporary directory with
fs.mkdtempSync, call plugin.writeBundle({} as any, { dir: tempDir } as any) or
the hook signature used in the plugin and verify expected files are written to
disk; for configureServer create a mock ViteDevServer object (with
use/middlewares or ws stubs as needed), call plugin.configureServer(server) and
assert the middleware or websocket handlers were registered. Ensure tests
reference EsCSS, plugin.buildStart, plugin.writeBundle and
plugin.configureServer so they exercise those hooks and clean up the temp
directory after assertions.
| it('load: devuelve CSS compilado para id con ?from=escss', () => { | ||
| const resolvedId = path.join(fixturesDir, 'estilos.css') + '?from=escss' | ||
| const result = plugin.load!(resolvedId) | ||
| expect(result).toBeDefined() | ||
| expect(result).toHaveProperty('code') | ||
| expect((result as { code: string }).code).toContain('display: flex') | ||
| expect((result as { code: string }).code).toContain('background-color') | ||
| expect((result as { code: string }).code).not.toContain('mostrar') | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cast repetido de result — extraer a variable tipada.
(result as { code: string }).code se repite tres veces. Asigne a una variable para mejorar la legibilidad.
Sugerencia
const result = plugin.load!(resolvedId)
expect(result).toBeDefined()
expect(result).toHaveProperty('code')
- expect((result as { code: string }).code).toContain('display: flex')
- expect((result as { code: string }).code).toContain('background-color')
- expect((result as { code: string }).code).not.toContain('mostrar')
+ const { code } = result as { code: string }
+ expect(code).toContain('display: flex')
+ expect(code).toContain('background-color')
+ expect(code).not.toContain('mostrar')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('load: devuelve CSS compilado para id con ?from=escss', () => { | |
| const resolvedId = path.join(fixturesDir, 'estilos.css') + '?from=escss' | |
| const result = plugin.load!(resolvedId) | |
| expect(result).toBeDefined() | |
| expect(result).toHaveProperty('code') | |
| expect((result as { code: string }).code).toContain('display: flex') | |
| expect((result as { code: string }).code).toContain('background-color') | |
| expect((result as { code: string }).code).not.toContain('mostrar') | |
| }) | |
| it('load: devuelve CSS compilado para id con ?from=escss', () => { | |
| const resolvedId = path.join(fixturesDir, 'estilos.css') + '?from=escss' | |
| const result = plugin.load!(resolvedId) | |
| expect(result).toBeDefined() | |
| expect(result).toHaveProperty('code') | |
| const { code } = result as { code: string } | |
| expect(code).toContain('display: flex') | |
| expect(code).toContain('background-color') | |
| expect(code).not.toContain('mostrar') | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vite-plugin-escss/test/vite-plugin-escss.test.ts` around lines 55 -
63, The test repeatedly casts result to { code: string }; assign the typed value
once to a local variable (e.g., const loaded = result as { code: string }) after
calling plugin.load!(resolvedId) and then use loaded.code in the three
assertions to improve readability; update the test around the
plugin.load!/resolvedId usage accordingly (retain same assertions but replace
repeated casts with the single typed variable).
| it('transformIndexHtml: sustituye link .escss por link al .css (archivo emitido en writeBundle)', () => { | ||
| const handler = | ||
| typeof plugin.transformIndexHtml === 'object' && | ||
| 'handler' in plugin.transformIndexHtml | ||
| ? plugin.transformIndexHtml.handler | ||
| : plugin.transformIndexHtml | ||
| expect(handler).toBeTypeOf('function') | ||
|
|
||
| const html = ` | ||
| <!DOCTYPE html> | ||
| <html> | ||
| <head> | ||
| <link rel="stylesheet" href="./estilos.escss"> | ||
| </head> | ||
| <body></body> | ||
| </html>` | ||
| const result = (handler as (html: string) => string)(html) | ||
| expect(result).not.toContain('href="./estilos.escss"') | ||
| expect(result).toContain('rel="stylesheet"') | ||
| expect(result).toContain('href="/assets/estilos.css"') | ||
| expect(result).not.toContain('<style>') | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test de transformIndexHtml no verifica que los assets se acumulen en escssAssetsToEmit.
El test verifica que el HTML se transforma correctamente, pero no comprueba que el asset compilado se haya registrado para emisión posterior en writeBundle. Sin esta verificación, el HTML podría apuntar a un .css que nunca se escribe a disco.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vite-plugin-escss/test/vite-plugin-escss.test.ts` around lines 65 -
86, The test for transformIndexHtml currently checks HTML output but doesn't
assert the plugin recorded the compiled asset; update the test to also assert
that the plugin's escssAssetsToEmit (the collection populated by
transformIndexHtml) contains an entry for 'estilos.escss' mapping to the
expected output filename (e.g., 'assets/estilos.css') so writeBundle will emit
it; locate where handler (from transformIndexHtml) is used and add an assertion
on plugin.escssAssetsToEmit (or the exact property name used in the plugin)
after calling handler to ensure the asset was registered for emission by
writeBundle.
1 participant
Summary by CodeRabbit
Notas de la versión
Nuevas Funcionalidades
Mejoras
Chores