From da975579b94b100f278d2b0ade0ffab76506d630 Mon Sep 17 00:00:00 2001 From: sapphi-red <49056869+sapphi-red@users.noreply.github.com> Date: Wed, 22 Nov 2023 20:12:16 +0900 Subject: [PATCH 1/4] feat(alias): add warning to avoid unintended duplicate modules --- packages/alias/src/index.ts | 15 +++- .../test/fixtures/folder/warn-importee.js | 1 + packages/alias/test/fixtures/warn.js | 2 + packages/alias/test/test.mjs | 72 +++++++++++++++++-- 4 files changed, 84 insertions(+), 6 deletions(-) create mode 100644 packages/alias/test/fixtures/folder/warn-importee.js create mode 100644 packages/alias/test/fixtures/warn.js diff --git a/packages/alias/src/index.ts b/packages/alias/src/index.ts index 8c5072571..51503848c 100755 --- a/packages/alias/src/index.ts +++ b/packages/alias/src/index.ts @@ -1,3 +1,5 @@ +import path from 'path'; + import type { Plugin } from 'rollup'; import type { ResolvedAlias, ResolverFunction, ResolverObject, RollupAliasOptions } from '../types'; @@ -97,7 +99,18 @@ export default function alias(options: RollupAliasOptions = {}): Plugin { updatedId, importer, Object.assign({ skipSelf: true }, resolveOptions) - ).then((resolved) => resolved || { id: updatedId }); + ).then((resolved) => { + if (resolved) return resolved; + + if (!path.isAbsolute(updatedId)) { + this.warn( + `rewrote ${importee} to ${updatedId} but was not an abolute path and was not handled by other plugins. ` + + `This will lead to duplicated modules for the same path. ` + + `To avoid duplicating modules, you should resolve to an absolute path.` + ); + } + return { id: updatedId }; + }); } }; } diff --git a/packages/alias/test/fixtures/folder/warn-importee.js b/packages/alias/test/fixtures/folder/warn-importee.js new file mode 100644 index 000000000..0f4e35a4a --- /dev/null +++ b/packages/alias/test/fixtures/folder/warn-importee.js @@ -0,0 +1 @@ +console.log() diff --git a/packages/alias/test/fixtures/warn.js b/packages/alias/test/fixtures/warn.js new file mode 100644 index 000000000..f655535b5 --- /dev/null +++ b/packages/alias/test/fixtures/warn.js @@ -0,0 +1,2 @@ +import '@/warn-importee.js' +import './folder/warn-importee.js' diff --git a/packages/alias/test/test.mjs b/packages/alias/test/test.mjs index 55a67aa0d..429fdd772 100755 --- a/packages/alias/test/test.mjs +++ b/packages/alias/test/test.mjs @@ -1,6 +1,7 @@ import path, { posix } from 'path'; import { fileURLToPath } from 'url'; import { createRequire } from 'module'; +import os from 'os'; import test from 'ava'; import { rollup } from 'rollup'; @@ -10,13 +11,16 @@ import alias from 'current-package'; const DIRNAME = fileURLToPath(new URL('.', import.meta.url)); +const isWindows = os.platform() === 'win32'; + /** * Helper function to test configuration with Rollup * @param plugins is an array of plugins for Rollup, they should include "alias" + * @param externalIds is an array of ids that will be external * @param tests is an array of pairs [source, importer] * @returns {Promise} */ -function resolveWithRollup(plugins, tests) { +function resolveWithRollup(plugins, externalIds, tests) { if (!plugins.find((p) => p.name === 'alias')) { throw new Error('`plugins` should include the alias plugin.'); } @@ -41,6 +45,7 @@ function resolveWithRollup(plugins, tests) { }, resolveId(id) { if (id === 'dummy-input') return id; + if (externalIds.includes(id)) return { id, external: true }; return null; }, load(id) { @@ -58,11 +63,12 @@ function resolveWithRollup(plugins, tests) { /** * Helper function to test configuration with Rollup and injected alias plugin * @param aliasOptions is a configuration for alias plugin + * @param externalIds is an array of ids that will be external * @param tests is an array of pairs [source, importer] * @returns {Promise} */ -function resolveAliasWithRollup(aliasOptions, tests) { - return resolveWithRollup([alias(aliasOptions)], tests); +function resolveAliasWithRollup(aliasOptions, externalIds, tests) { + return resolveWithRollup([alias(aliasOptions)], externalIds, tests); } test('type', (t) => { @@ -90,6 +96,7 @@ test('Simple aliasing (array)', (t) => { find: './local', replacement: 'global' } ] }, + ['bar', 'paradise', 'global'], [ { source: 'foo', importer: '/src/importer.js' }, { source: 'pony', importer: '/src/importer.js' }, @@ -106,6 +113,7 @@ test('Simple aliasing (object)', (t) => './local': 'global' } }, + ['bar', 'paradise', 'global'], [ { source: 'foo', importer: '/src/importer.js' }, { source: 'pony', importer: '/src/importer.js' }, @@ -125,6 +133,7 @@ test('RegExp aliasing', (t) => { find: /^test\/$/, replacement: 'this/is/strict' } ] }, + ['fooooooooobar2019', 'i/am/a/barbie/girl', 'this/is/strict'], [ { source: 'fooooooooobar', @@ -150,6 +159,7 @@ test('Will not confuse modules with similar names', (t) => { find: './foo', replacement: 'bar' } ] }, + [], [ { source: 'foo2', importer: '/src/importer.js' }, { @@ -168,6 +178,7 @@ test('Alias entry file', (t) => { entries: [{ find: 'abacaxi', replacement: './abacaxi' }] }, + ['./abacaxi/entry.js'], // eslint-disable-next-line no-undefined [{ source: 'abacaxi/entry.js' }] ).then((result) => t.deepEqual(result, ['./abacaxi/entry.js']))); @@ -177,10 +188,16 @@ test('i/am/a/file', (t) => { entries: [{ find: 'resolve', replacement: 'i/am/a/file' }] }, + ['i/am/a/file'], [{ source: 'resolve', importer: '/src/import.js' }] ).then((result) => t.deepEqual(result, ['i/am/a/file']))); -test('Windows absolute path aliasing', (t) => +test('Windows absolute path aliasing', (t) => { + if (!isWindows) { + t.deepEqual(1, 1); + return; + } + resolveAliasWithRollup( { entries: [ @@ -190,13 +207,15 @@ test('Windows absolute path aliasing', (t) => } ] }, + [], [ { source: 'resolve', importer: posix.resolve(DIRNAME, './fixtures/index.js') } ] - ).then((result) => t.deepEqual(result, ['E:\\react\\node_modules\\fbjs\\lib\\warning']))); + ).then((result) => t.deepEqual(result, ['E:\\react\\node_modules\\fbjs\\lib\\warning'])); +}); /** * Helper function to get moduleIDs from final Rollup bundle @@ -276,6 +295,7 @@ test('Global customResolver function', (t) => { ], customResolver: () => customResult }, + [], [ { source: 'test', @@ -300,6 +320,7 @@ test('Local customResolver function', (t) => { ], customResolver: () => customResult }, + [], [ { source: 'test', @@ -322,6 +343,7 @@ test('Global customResolver plugin-like object', (t) => { ], customResolver: { resolveId: () => customResult } }, + [], [ { source: 'test', @@ -348,6 +370,7 @@ test('Local customResolver plugin-like object', (t) => { ], customResolver: { resolveId: () => customResult } }, + [], [ { source: 'test', @@ -373,6 +396,7 @@ test('supports node-resolve as a custom resolver', (t) => ], customResolver: nodeResolvePlugin() }, + [], [ { source: 'local-resolver', @@ -450,6 +474,7 @@ test('Forwards isEntry and custom options to a custom resolver', (t) => { return args[0]; } }, + [], [ { source: 'entry', importer: '/src/importer.js', options: { isEntry: true } }, { @@ -497,9 +522,15 @@ test('Forwards isEntry and custom options to other plugins', (t) => { name: 'test', resolveId(...args) { resolverCalls.push(args); + + if (['entry-point', 'non-entry-point'].includes(args[0])) { + return { id: args[0], external: true }; + } + return null; } } ], + [], [ { source: 'entry', importer: '/src/importer.js', options: { isEntry: true } }, { @@ -558,6 +589,7 @@ test('CustomResolver plugin-like object with buildStart', (t) => { buildStart: () => (count.option += 1) } }, + [], [] ).then(() => t.deepEqual(count, { @@ -608,3 +640,33 @@ test('Works as CJS plugin', async (t) => { ) ); }); + +test('show warning for non-absolute non-plugin resolved id', async (t) => { + const warnList = []; + await rollup({ + input: './test/fixtures/warn.js', + plugins: [ + alias({ + entries: [ + { + find: '@', + replacement: path.relative(process.cwd(), path.join(DIRNAME, './fixtures/folder')) + } + ] + }) + ], + onwarn(log) { + warnList.push(log); + } + }); + t.deepEqual(warnList, [ + { + message: + 'rewrote @/warn-importee.js to test/fixtures/folder/warn-importee.js but was not an abolute path and was not handled by other plugins. ' + + 'This will lead to duplicated modules for the same path. ' + + 'To avoid duplicating modules, you should resolve to an absolute path.', + code: 'PLUGIN_WARNING', + plugin: 'alias' + } + ]); +}); From a8e419b142cdb9ca700a946a60417aea2ae4c0f9 Mon Sep 17 00:00:00 2001 From: sapphi-red <49056869+sapphi-red@users.noreply.github.com> Date: Wed, 22 Nov 2023 21:03:56 +0900 Subject: [PATCH 2/4] test: normalize slash --- packages/alias/test/test.mjs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/alias/test/test.mjs b/packages/alias/test/test.mjs index 429fdd772..10b0ddf99 100755 --- a/packages/alias/test/test.mjs +++ b/packages/alias/test/test.mjs @@ -656,7 +656,8 @@ test('show warning for non-absolute non-plugin resolved id', async (t) => { }) ], onwarn(log) { - warnList.push(log); + const formattedLog = { ...log, message: log.message.replace(/\//g, '/') }; + warnList.push(formattedLog); } }); t.deepEqual(warnList, [ From 8436c3d5bfe8d31ecb154b095919d9fc486f4cab Mon Sep 17 00:00:00 2001 From: sapphi-red <49056869+sapphi-red@users.noreply.github.com> Date: Wed, 22 Nov 2023 21:07:55 +0900 Subject: [PATCH 3/4] test: normalize slash correctly --- packages/alias/test/test.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/alias/test/test.mjs b/packages/alias/test/test.mjs index 10b0ddf99..8c8f0cd86 100755 --- a/packages/alias/test/test.mjs +++ b/packages/alias/test/test.mjs @@ -656,7 +656,7 @@ test('show warning for non-absolute non-plugin resolved id', async (t) => { }) ], onwarn(log) { - const formattedLog = { ...log, message: log.message.replace(/\//g, '/') }; + const formattedLog = { ...log, message: log.message.replace(/\\/g, '/') }; warnList.push(formattedLog); } }); From 089679103d741952d5ae9798ea6b750774f1fe29 Mon Sep 17 00:00:00 2001 From: sapphi-red <49056869+sapphi-red@users.noreply.github.com> Date: Wed, 22 Nov 2023 21:11:49 +0900 Subject: [PATCH 4/4] test: return promise --- packages/alias/test/test.mjs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/alias/test/test.mjs b/packages/alias/test/test.mjs index 8c8f0cd86..2f0be4eab 100755 --- a/packages/alias/test/test.mjs +++ b/packages/alias/test/test.mjs @@ -195,10 +195,10 @@ test('i/am/a/file', (t) => test('Windows absolute path aliasing', (t) => { if (!isWindows) { t.deepEqual(1, 1); - return; + return null; } - resolveAliasWithRollup( + return resolveAliasWithRollup( { entries: [ {