-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
fix(build): ensure amd bundles request require to be injected
#20861
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
Changes from 9 commits
835e172
4eaac92
e890074
afbbbcf
806edb7
6f6d3c4
560154b
2a2b2e8
87b6ff3
74c9da4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import { describe, expect, test } from 'vitest' | ||
| import { completeAmdWrapPlugin } from '../../plugins/completeAmdWrap' | ||
|
|
||
| async function createCompleteAmdWrapPluginRenderChunk() { | ||
| const instance = completeAmdWrapPlugin() | ||
|
|
||
| return async (code: string) => { | ||
| // @ts-expect-error transform.handler should exist | ||
| const result = await instance.renderChunk.call(instance, code, 'foo.ts', { | ||
| format: 'amd', | ||
| }) | ||
| return result?.code || result | ||
| } | ||
| } | ||
|
|
||
| describe('completeAmdWrapPlugin', async () => { | ||
| const renderChunk = await createCompleteAmdWrapPluginRenderChunk() | ||
|
|
||
| describe('adds require parameter', async () => { | ||
| test('without other dependencies', async () => { | ||
| expect( | ||
| await renderChunk('define((function() { } ))'), | ||
| ).toMatchInlineSnapshot(`"define(["require"], (function(require) { } ))"`) | ||
| }) | ||
|
|
||
| test('with other dependencies', async () => { | ||
| expect( | ||
| await renderChunk( | ||
| 'define(["vue", "vue-router"], function(vue, vueRouter) { } ))', | ||
| ), | ||
| ).toMatchInlineSnapshot( | ||
| `"define(["require", "vue", "vue-router"], (function(require, vue, vueRouter) { } ))"`, | ||
| ) | ||
| }) | ||
|
|
||
| test("only if require isn't injected already", async () => { | ||
| expect( | ||
| await renderChunk('define(["require"], function(require) { } ))'), | ||
| ).toMatchInlineSnapshot(`"define(["require"], (function(require) { } ))"`) | ||
|
|
||
| expect( | ||
| await renderChunk(`define(['require'], function(require) { } ))`), | ||
| ).toMatchInlineSnapshot(`"define(['require'], (function(require) { } ))"`) | ||
| }) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| import type { Plugin } from '../plugin' | ||
|
|
||
| /** | ||
| * ensure amd bundles request `require` to be injected | ||
| */ | ||
| export function completeAmdWrapPlugin(): Plugin { | ||
| const AmdWrapRE = | ||
| /\bdefine\((?:\s*\[([^\]]*)\],)?\s*(?:\(\s*)?function\s*\(([^)]*)\)\s*\{/g | ||
|
|
||
| return { | ||
| name: 'vite:force-amd-wrap-require', | ||
| renderChunk(code, _chunk, opts) { | ||
| if (opts.format !== 'amd') return | ||
|
|
||
| return { | ||
| code: code.replace(AmdWrapRE, (_, deps, params) => { | ||
| if (deps?.includes(`"require"`) || deps?.includes(`'require'`)) { | ||
| return `define([${deps}], (function(${params}) {` | ||
| } | ||
|
|
||
| const newDeps = deps ? `"require", ${deps}` : '"require"' | ||
| const newParams = params.trim() ? `require, ${params}` : 'require' | ||
|
|
||
| return `define([${newDeps}], (function(${newParams}) {` | ||
| }), | ||
| map: null, // no need to generate sourcemap as no mapping exists for the wrapper | ||
| } | ||
| }, | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import { describe, expect, test } from 'vitest' | ||
| import { browserLogs, getBg, isBuild, page } from '~utils' | ||
|
|
||
| describe.runIf(isBuild)('build', () => { | ||
| test('should have no 404s', async () => { | ||
| await page.waitForLoadState('networkidle') | ||
| browserLogs.forEach((msg) => { | ||
| expect(msg).not.toMatch('404') | ||
| }) | ||
| }) | ||
|
|
||
| test('asset url is correct with `base: "."`', async () => { | ||
| await expect | ||
| .poll(() => getBg('.assets')) | ||
| .toMatch(/\/assets\/asset-[-\w]{8}\.png/) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import asset from './asset.png' | ||
|
|
||
| export default function pluginMain() { | ||
| return asset | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { | ||
| "scripts": { | ||
| "dev": "vite", | ||
| "build": "vite build", | ||
| "preview": "vite preview" | ||
| }, | ||
| "dependencies": { | ||
| "requirejs": "^2.3.7" | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason this file is in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is because this file is not part of the build (the entrypoint is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, but it feels a bit weird to be building a Vite app this way. Usually the build output format doesn't affect dev, but I suppose it's fine as a test setup for now. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| <!doctype html> | ||
| <html lang="en"> | ||
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <title>amd</title> | ||
| </head> | ||
| <body> | ||
| <h1>AMD</h1> | ||
| <p>An image should be shown below</p> | ||
| <div | ||
| class="assets" | ||
| style="background-size: cover; height: 100px; width: 100px" | ||
| ></div> | ||
|
|
||
| <script src="/npm/requirejs.js"></script> | ||
| <script type="text/javascript"> | ||
| requirejs( | ||
| ['assets/plugin.js'], | ||
| (plugin) => { | ||
| const url = plugin() | ||
| document.querySelector('.assets').style.backgroundImage = | ||
| `url(${url})` | ||
| }, | ||
| (err) => { | ||
| console.error('Error loading plugin', err) | ||
| }, | ||
| ) | ||
| </script> | ||
| </body> | ||
| </html> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| import fs from 'node:fs/promises' | ||
| import path from 'node:path' | ||
| import { type Connect, defineConfig } from 'vite' | ||
|
|
||
| export default defineConfig({ | ||
| base: './', | ||
| build: { | ||
| outDir: 'dist/nested', | ||
| rollupOptions: { | ||
| preserveEntrySignatures: 'strict', | ||
| input: { | ||
| plugin: path.resolve(import.meta.dirname, './index.ts'), | ||
| }, | ||
| output: { | ||
| format: 'amd', | ||
| entryFileNames: 'assets/[name].js', | ||
| }, | ||
| }, | ||
| }, | ||
| plugins: [ | ||
| { | ||
| name: 'serve-npm-code-directly', | ||
| configureServer({ middlewares }) { | ||
| middlewares.use(serveNpmCodeDirectlyMiddleware) | ||
| }, | ||
| configurePreviewServer({ middlewares }) { | ||
| middlewares.use(serveNpmCodeDirectlyMiddleware) | ||
| }, | ||
| }, | ||
| ], | ||
| appType: 'mpa', // to cause 404 for incorrect URLs | ||
| }) | ||
|
|
||
| const npmDirectServeConfig = { | ||
| '/npm/requirejs.js': 'requirejs/require.js', | ||
| } | ||
| const serveNpmCodeDirectlyMiddleware: Connect.NextHandleFunction = async ( | ||
| req, | ||
| res, | ||
| next, | ||
| ) => { | ||
| for (const [url, file] of Object.entries(npmDirectServeConfig)) { | ||
| if (req.originalUrl === url) { | ||
| const code = await fs.readFile( | ||
| new URL(`./node_modules/${file}`, import.meta.url), | ||
| ) | ||
| res.setHeader('Content-Type', 'text/javascript') | ||
| res.end(code) | ||
| return | ||
| } | ||
| } | ||
| next() | ||
| } |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.