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(virtual): incorrect import path #1

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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
4 changes: 4 additions & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,7 @@ packages/typescript/test/fixtures/syntax-error

# temporary workaround for eslint bug where package.json is a directory
packages/node-resolve/test/fixtures/package-json-in-path

# temporary workaround for TypeScript as it doesn't support "Arbitrary module namespace identifier names"
# https://github.com/microsoft/TypeScript/issues/40594
packages/json/test/fixtures/arbitrary/main.js
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"lint:js": "eslint --cache packages scripts shared util --ext .js,.ts,.mjs",
"lint:json": "prettier --write .github/**/*.yml **/tsconfig.json tsconfig.*.json pnpm-workspace.yaml",
"lint:package": "prettier --write **/package.json",
"package:release": "versioner --stripShortName='^@.+/plugin-' --target",
"package:release": "versioner --stripShortName='^@.+/(plugin-)?' --target",
"preinstall": "node scripts/disallow-npm.js",
"prepare": "husky install",
"prettier": "prettier --write .",
Expand Down
8 changes: 8 additions & 0 deletions packages/alias/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
# @rollup/plugin-alias ChangeLog

## v5.1.0

_2023-11-25_

### Features

- feat: add warning to avoid unintended duplicate modules (#1634)

## v5.0.1

_2023-10-05_
Expand Down
2 changes: 1 addition & 1 deletion packages/alias/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@rollup/plugin-alias",
"version": "5.0.1",
"version": "5.1.0",
"publishConfig": {
"access": "public"
},
Expand Down
15 changes: 14 additions & 1 deletion packages/alias/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import path from 'path';

import type { Plugin } from 'rollup';

import type { ResolvedAlias, ResolverFunction, ResolverObject, RollupAliasOptions } from '../types';
Expand Down Expand Up @@ -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 };
});
}
};
}
1 change: 1 addition & 0 deletions packages/alias/test/fixtures/folder/warn-importee.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log()
2 changes: 2 additions & 0 deletions packages/alias/test/fixtures/warn.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import '@/warn-importee.js'
import './folder/warn-importee.js'
75 changes: 69 additions & 6 deletions packages/alias/test/test.mjs
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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<unknown>}
*/
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.');
}
Expand All @@ -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) {
Expand All @@ -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<unknown>}
*/
function resolveAliasWithRollup(aliasOptions, tests) {
return resolveWithRollup([alias(aliasOptions)], tests);
function resolveAliasWithRollup(aliasOptions, externalIds, tests) {
return resolveWithRollup([alias(aliasOptions)], externalIds, tests);
}

test('type', (t) => {
Expand Down Expand Up @@ -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' },
Expand All @@ -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' },
Expand All @@ -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',
Expand All @@ -150,6 +159,7 @@ test('Will not confuse modules with similar names', (t) =>
{ find: './foo', replacement: 'bar' }
]
},
[],
[
{ source: 'foo2', importer: '/src/importer.js' },
{
Expand All @@ -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'])));
Expand All @@ -177,11 +188,17 @@ 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) =>
resolveAliasWithRollup(
test('Windows absolute path aliasing', (t) => {
if (!isWindows) {
t.deepEqual(1, 1);
return null;
}

return resolveAliasWithRollup(
{
entries: [
{
Expand All @@ -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
Expand Down Expand Up @@ -276,6 +295,7 @@ test('Global customResolver function', (t) => {
],
customResolver: () => customResult
},
[],
[
{
source: 'test',
Expand All @@ -300,6 +320,7 @@ test('Local customResolver function', (t) => {
],
customResolver: () => customResult
},
[],
[
{
source: 'test',
Expand All @@ -322,6 +343,7 @@ test('Global customResolver plugin-like object', (t) => {
],
customResolver: { resolveId: () => customResult }
},
[],
[
{
source: 'test',
Expand All @@ -348,6 +370,7 @@ test('Local customResolver plugin-like object', (t) => {
],
customResolver: { resolveId: () => customResult }
},
[],
[
{
source: 'test',
Expand All @@ -373,6 +396,7 @@ test('supports node-resolve as a custom resolver', (t) =>
],
customResolver: nodeResolvePlugin()
},
[],
[
{
source: 'local-resolver',
Expand Down Expand Up @@ -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 } },
{
Expand Down Expand Up @@ -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 } },
{
Expand Down Expand Up @@ -558,6 +589,7 @@ test('CustomResolver plugin-like object with buildStart', (t) => {
buildStart: () => (count.option += 1)
}
},
[],
[]
).then(() =>
t.deepEqual(count, {
Expand Down Expand Up @@ -608,3 +640,34 @@ 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) {
const formattedLog = { ...log, message: log.message.replace(/\\/g, '/') };
warnList.push(formattedLog);
}
});
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'
}
]);
});
16 changes: 16 additions & 0 deletions packages/dynamic-import-vars/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,21 @@
# @rollup/plugin-dynamic-import-vars ChangeLog

## v2.1.2

_2023-11-28_

### Bugfixes

- fix: Allow a "no files found" error to be emitted as warning (#1625)

## v2.1.1

_2023-11-25_

### Bugfixes

- fix: escape special glob characters (#1636)

## v2.1.0

_2023-10-25_
Expand Down
2 changes: 2 additions & 0 deletions packages/dynamic-import-vars/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ Default: `false`

By default, the plugin will not throw errors when target files are not found. Setting this option to true will result in errors thrown when encountering files which don't exist.

⚠️ _Important:_ Enabling this option when `warnOnError` is set to `true` will result in a warning and _not_ an error

#### `warnOnError`

Type: `Boolean`<br>
Expand Down
2 changes: 1 addition & 1 deletion packages/dynamic-import-vars/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@rollup/plugin-dynamic-import-vars",
"version": "2.1.0",
"version": "2.1.2",
"publishConfig": {
"access": "public"
},
Expand Down
5 changes: 4 additions & 1 deletion packages/dynamic-import-vars/src/dynamic-import-to-glob.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
import path from 'path';

import fastGlob from 'fast-glob';

export class VariableDynamicImportError extends Error {}

/* eslint-disable-next-line no-template-curly-in-string */
const example = 'For example: import(`./foo/${bar}.js`).';

function sanitizeString(str) {
if (str === '') return str;
if (str.includes('*')) {
throw new VariableDynamicImportError('A dynamic import cannot contain * characters.');
}
return str;
return fastGlob.escapePath(str);
}

function templateLiteralToGlob(node) {
Expand Down
11 changes: 7 additions & 4 deletions packages/dynamic-import-vars/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,14 @@ function dynamicImportVariables({ include, exclude, warnOnError, errorWhenNoFile
);

if (errorWhenNoFilesFound && paths.length === 0) {
this.error(
new Error(
`No files found in ${glob} when trying to dynamically load concatted string from ${id}`
)
const error = new Error(
`No files found in ${glob} when trying to dynamically load concatted string from ${id}`
);
if (warnOnError) {
this.warn(error);
} else {
this.error(error);
}
}

// create magic string if it wasn't created already
Expand Down
Loading