Skip to content
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

fix(minify): set the default value of esbuild's legalComments option to external #18104

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/vite/src/node/plugins/css.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1885,7 +1885,8 @@ function resolveMinifyCssEsbuildOptions(
logLevel: options.logLevel,
logLimit: options.logLimit,
logOverride: options.logOverride,
legalComments: options.legalComments,
legalComments:
options.legalComments === 'linked' ? 'external' : options.legalComments,
}

if (
Expand Down
42 changes: 42 additions & 0 deletions packages/vite/src/node/plugins/esbuild.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export interface ESBuildOptions extends TransformOptions {
* This option is not respected. Use `build.minify` instead.
*/
minify?: never
legalComments?: 'none' | 'inline' | 'eof' | 'linked' | 'external'
}

export type ESBuildTransformResult = Omit<TransformResult, 'map'> & {
Expand Down Expand Up @@ -165,6 +166,7 @@ export async function transformWithEsbuild(
}

const resolvedOptions: TransformOptions = {
legalComments: 'linked',
sourcemap: true,
// ensure source file name contains full query
sourcefile: filename,
Expand All @@ -173,6 +175,10 @@ export async function transformWithEsbuild(
tsconfigRaw,
}

if (resolvedOptions.legalComments === 'linked') {
resolvedOptions.legalComments = 'external'
}

// Some projects in the ecosystem are calling this function with an ESBuildOptions
// object and esbuild throws an error for extra fields
// @ts-expect-error include exists in ESBuildOptions
Expand Down Expand Up @@ -303,6 +309,8 @@ const rollupToEsbuildFormatMap: Record<
}

export const buildEsbuildPlugin = (config: ResolvedConfig): Plugin => {
const collectLegalComments: string[] = []

return {
name: 'vite:esbuild-transpile',
async renderChunk(code, chunk, opts) {
Expand All @@ -319,6 +327,10 @@ export const buildEsbuildPlugin = (config: ResolvedConfig): Plugin => {

const res = await transformWithEsbuild(code, chunk.fileName, options)

if (res.legalComments) {
collectLegalComments.push(res.legalComments)
}

if (config.build.lib) {
// #7188, esbuild adds helpers out of the UMD and IIFE wrappers, and the
// names are minified potentially causing collision with other globals.
Expand Down Expand Up @@ -348,6 +360,36 @@ export const buildEsbuildPlugin = (config: ResolvedConfig): Plugin => {

return res
},

generateBundle(options, bundle) {
if (collectLegalComments.length && options.dir) {
const referenceId = this.emitFile({
name: 'LEGAL.txt',
type: 'asset',
source: collectLegalComments.join('\n'),
})

if (config.esbuild && config.esbuild.legalComments === 'linked') {
const legalCommentsFileName = this.getFileName(referenceId)
const linkedComments = `/*! For license information please see ${legalCommentsFileName} */`

for (const file in bundle) {
const chunk = bundle[file]

if (
chunk.fileName.endsWith('.css') ||
chunk.fileName.endsWith('.js')
) {
if (chunk.type === 'asset') {
chunk.source += linkedComments
} else if (chunk.type === 'chunk') {
chunk.code += linkedComments
}
}
}
}
}
},
}
}

Expand Down
79 changes: 70 additions & 9 deletions playground/minify/__tests__/minify.spec.ts
Original file line number Diff line number Diff line change
@@ -1,21 +1,82 @@
import fs from 'node:fs'
import path from 'node:path'
import { expect, test } from 'vitest'
import { describe, expect, test } from 'vitest'
import { isBuild, readFile, testDir } from '~utils'

test.runIf(isBuild)('no minifySyntax', () => {
const assetsDir = path.resolve(testDir, 'dist/assets')
function getJsAndCssContent(assetsDir: string) {
const files = fs.readdirSync(assetsDir)

const jsFile = files.find((f) => f.endsWith('.js'))
const jsContent = readFile(path.resolve(assetsDir, jsFile))
const jsContent = readFile(path.resolve(assetsDir, jsFile)).trim()

const cssFile = files.find((f) => f.endsWith('.css'))
const cssContent = readFile(path.resolve(assetsDir, cssFile))
const cssContent = readFile(path.resolve(assetsDir, cssFile)).trim()

expect(jsContent).toContain('{console.log("hello world")}')
expect(jsContent).not.toContain('/*! explicit comment */')
return { jsContent, cssContent }
}

expect(cssContent).toContain('color:#ff0000')
expect(cssContent).not.toContain('/*! explicit comment */')
function getLegalFileContent(assetsDir: string) {
const files = fs.readdirSync(assetsDir)

for (const fileName of files) {
if (fileName.startsWith('LEGAL')) {
return readFile(path.join(assetsDir, fileName))
}
}
}

const legalComments = '/*! explicit comment */'

describe.runIf(isBuild)('minify', () => {
test('Do not preserve any legal comments', () => {
const { jsContent, cssContent } = getJsAndCssContent(
path.resolve(testDir, 'dist/none/assets'),
)

expect(jsContent).toContain('{console.log("hello world")}')
expect(jsContent).not.toContain(legalComments)
expect(cssContent).toContain('color:#ff0000')
expect(cssContent).not.toContain(legalComments)
})

test('Preserve legal comments', () => {
const { jsContent, cssContent } = getJsAndCssContent(
path.resolve(testDir, 'dist/inline/assets'),
)

expect(jsContent).toContain(legalComments)
expect(cssContent).toContain(legalComments)
})

test('Move all legal comments to the end of the file.', () => {
const { jsContent, cssContent } = getJsAndCssContent(
path.resolve(testDir, 'dist/eof/assets'),
)

expect(jsContent.endsWith(legalComments)).toBeTruthy()
expect(cssContent.endsWith(legalComments)).toBeTruthy()
})

test('Move all legal comments to a LEGAL.txt file but to not link to them.', () => {
const assetsPath = path.resolve(testDir, 'dist/external/assets')
const { jsContent, cssContent } = getJsAndCssContent(assetsPath)
const legaContent = getLegalFileContent(assetsPath)

expect(jsContent).not.toContain(legalComments)
expect(cssContent).not.toContain(legalComments)
expect(legaContent).toContain(legalComments)
})

test('Move all legal comments to a LEGAL.txt file and link to them with a comment.', () => {
const linkedCommentsPre = 'For license information please see'
const assetsPath = path.resolve(testDir, 'dist/linked/assets')
const { jsContent, cssContent } = getJsAndCssContent(assetsPath)
const legaContent = getLegalFileContent(assetsPath)

expect(jsContent).not.toContain(legalComments)
expect(jsContent.includes(linkedCommentsPre)).toBeTruthy()
expect(cssContent).not.toContain(legalComments)
expect(cssContent.includes(linkedCommentsPre)).toBeTruthy()
expect(legaContent).toContain(legalComments)
})
})
47 changes: 47 additions & 0 deletions playground/minify/__tests__/serve.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// this is automatically detected by playground/vitestSetup.ts and will replace
// the default e2e test serve behavior

import path from 'node:path'
import { ports, rootDir } from '~utils'

export const port = ports.lib

export async function serve() {
const { build } = await import('vite')
await build({
root: rootDir,
configFile: path.resolve(__dirname, '../vite.legal-comments-eof.config.js'),
})

await build({
root: rootDir,
configFile: path.resolve(
__dirname,
'../vite.legal-comments-external.config.js',
),
})

await build({
root: rootDir,
configFile: path.resolve(
__dirname,
'../vite.legal-comments-inline.config.js',
),
})

await build({
root: rootDir,
configFile: path.resolve(
__dirname,
'../vite.legal-comments-none.config.js',
),
})

await build({
root: rootDir,
configFile: path.resolve(
__dirname,
'../vite.legal-comments-linked.config.js',
),
})
}
11 changes: 11 additions & 0 deletions playground/minify/vite.legal-comments-eof.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { defineConfig } from 'vite'

export default defineConfig({
esbuild: {
legalComments: 'eof',
minifySyntax: false,
},
build: {
outDir: 'dist/eof',
},
})
11 changes: 11 additions & 0 deletions playground/minify/vite.legal-comments-external.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { defineConfig } from 'vite'

export default defineConfig({
esbuild: {
legalComments: 'external',
minifySyntax: false,
},
build: {
outDir: 'dist/external',
},
})
11 changes: 11 additions & 0 deletions playground/minify/vite.legal-comments-inline.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { defineConfig } from 'vite'

export default defineConfig({
esbuild: {
legalComments: 'inline',
minifySyntax: false,
},
build: {
outDir: 'dist/inline',
},
})
11 changes: 11 additions & 0 deletions playground/minify/vite.legal-comments-linked.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { defineConfig } from 'vite'

export default defineConfig({
esbuild: {
legalComments: 'linked',
minifySyntax: false,
},
build: {
outDir: 'dist/linked',
},
})
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,7 @@ export default defineConfig({
legalComments: 'none',
minifySyntax: false,
},
build: {
outDir: 'dist/none',
},
})