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: more types #4361

Merged
merged 27 commits into from Jan 28, 2022
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
5cb4167
remove type assertion
dnalborczyk Jan 23, 2022
ffe2517
use type imports
dnalborczyk Jan 23, 2022
1db67e0
simplify creating Set
dnalborczyk Jan 23, 2022
891824e
use ternary to assign type (remove any)
dnalborczyk Jan 23, 2022
a50cde0
use readonly, add set types, use type imports
dnalborczyk Jan 23, 2022
31e2bf4
use more readonly
dnalborczyk Jan 23, 2022
b44b46b
use type inference
dnalborczyk Jan 23, 2022
58fa511
stricter types, use more readonly
dnalborczyk Jan 23, 2022
62538d5
remove type assertion + comment
dnalborczyk Jan 23, 2022
0a584b8
use const + new var
dnalborczyk Jan 23, 2022
6bb4195
add return types
dnalborczyk Jan 23, 2022
372a8fd
make plugincontexts readonly, simplify construction
dnalborczyk Jan 23, 2022
9140a2e
more readonly, type imports
dnalborczyk Jan 23, 2022
b841c99
add return types, fix return values
dnalborczyk Jan 23, 2022
fe7e0c4
combine if/else
dnalborczyk Jan 23, 2022
e331522
consistency: use arrow functions to pass 'this' context
dnalborczyk Jan 23, 2022
437e90b
consistency: use more arrow functions to pass this context
dnalborczyk Jan 24, 2022
7e87c36
Revert "consistency: use more arrow functions to pass this context"
dnalborczyk Jan 26, 2022
52d4b21
Revert "consistency: use arrow functions to pass 'this' context"
dnalborczyk Jan 26, 2022
8cf37cb
Revert "add return types, fix return values"
dnalborczyk Jan 26, 2022
e75e854
Merge branch 'master' into more-type-fixes
dnalborczyk Jan 26, 2022
488a386
use type imports
dnalborczyk Jan 28, 2022
9e986e6
Merge branch 'more-type-fixes' of https://github.com/dnalborczyk/roll…
dnalborczyk Jan 28, 2022
08bec13
use more readonly
dnalborczyk Jan 28, 2022
a96d407
replace any with unknown
dnalborczyk Jan 28, 2022
196ad88
use as const
dnalborczyk Jan 28, 2022
2c47163
Merge branch 'master' into more-type-fixes
lukastaegert Jan 28, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 16 additions & 14 deletions build-plugins/generate-license-file.ts
@@ -1,24 +1,24 @@
import { readFileSync, writeFileSync } from 'fs';
import { PluginImpl } from 'rollup';
import license, { Dependency, Person } from 'rollup-plugin-license';
import type { PluginImpl } from 'rollup';
import license, { type Dependency, type Person } from 'rollup-plugin-license';

function generateLicenseFile(dependencies: Dependency[]) {
function generateLicenseFile(dependencies: readonly Dependency[]) {
const coreLicense = readFileSync('LICENSE-CORE.md');
const licenses = new Set();
const dependencyLicenseTexts = dependencies
const licenses = new Set<string>();
const dependencyLicenseTexts = Array.from(dependencies)
.sort(({ name: nameA }, { name: nameB }) => (nameA! > nameB! ? 1 : -1))
.map(({ name, license, licenseText, author, maintainers, contributors, repository }) => {
let text = `## ${name}\n`;
if (license) {
text += `License: ${license}\n`;
}
const names = new Set();
if (author && author.name) {
const names = new Set<string>();
if (author?.name) {
names.add(author.name);
}
// TODO there is an inconsistency in the rollup-plugin-license types
for (const person of contributors.concat(maintainers as unknown as Person[])) {
if (person && person.name) {
if (person?.name) {
names.add(person.name);
}
}
Expand All @@ -39,7 +39,7 @@ function generateLicenseFile(dependencies: Dependency[]) {
.join('\n') +
'\n';
}
licenses.add(license);
licenses.add(license!);
return text;
})
.join('\n---------------------------------------\n\n');
Expand All @@ -59,16 +59,18 @@ function generateLicenseFile(dependencies: Dependency[]) {
}
}

export default function getLicenseHandler(): {
interface LicenseHandler {
collectLicenses: PluginImpl;
writeLicense: PluginImpl;
} {
const licenses = new Map();
}

export default function getLicenseHandler(): LicenseHandler {
const licenses = new Map<string, Dependency>();
return {
collectLicenses() {
function addLicenses(dependencies: Dependency[]) {
function addLicenses(dependencies: readonly Dependency[]) {
for (const dependency of dependencies) {
licenses.set(dependency.name, dependency);
licenses.set(dependency.name!, dependency);
}
}

Expand Down
6 changes: 3 additions & 3 deletions cli/run/loadConfigFile.ts
Expand Up @@ -2,14 +2,14 @@ import { realpathSync } from 'fs';
import { extname, isAbsolute } from 'path';
import { pathToFileURL } from 'url';
import * as rollup from '../../src/node-entry';
import { MergedRollupOptions } from '../../src/rollup/types';
import type { MergedRollupOptions } from '../../src/rollup/types';
import { bold } from '../../src/utils/colors';
import { error } from '../../src/utils/error';
import { mergeOptions } from '../../src/utils/options/mergeOptions';
import { GenericConfigObject } from '../../src/utils/options/options';
import type { GenericConfigObject } from '../../src/utils/options/options';
import relativeId from '../../src/utils/relativeId';
import { stderr } from '../logging';
import batchWarnings, { BatchWarnings } from './batchWarnings';
import batchWarnings, { type BatchWarnings } from './batchWarnings';
import { addCommandPluginsToInputOptions, addPluginsFromCommandOption } from './commandPlugins';

function supportsNativeESM(): boolean {
Expand Down
2 changes: 1 addition & 1 deletion cli/run/resetScreen.ts
@@ -1,4 +1,4 @@
import { MergedRollupOptions } from '../../src/rollup/types';
import type { MergedRollupOptions } from '../../src/rollup/types';
import { stderr } from '../logging';

const CLEAR_SCREEN = '\u001Bc';
Expand Down
2 changes: 1 addition & 1 deletion cli/run/stdin.ts
@@ -1,4 +1,4 @@
import { Plugin } from '../../src/rollup/types';
import type { Plugin } from '../../src/rollup/types';

export const stdinName = '-';

Expand Down
8 changes: 4 additions & 4 deletions cli/run/waitForInput.ts
@@ -1,5 +1,5 @@
import { PluginContext } from 'rollup';
import { NormalizedInputOptions, Plugin } from '../../src/rollup/types';
import type { PluginContext } from 'rollup';
import type { NormalizedInputOptions, Plugin } from '../../src/rollup/types';
import { bold } from '../../src/utils/colors';
import { stderr } from '../logging';

Expand All @@ -8,9 +8,9 @@ export function waitForInputPlugin(): Plugin {
async buildStart(this: PluginContext, options: NormalizedInputOptions) {
const inputSpecifiers = Array.isArray(options.input)
? options.input
: Object.keys(options.input as { [entryAlias: string]: string });
: Object.keys(options.input);

let lastAwaitedSpecifier = null;
let lastAwaitedSpecifier: string | null = null;
checkSpecifiers: while (true) {
for (const specifier of inputSpecifiers) {
if ((await this.resolve(specifier)) === null) {
Expand Down
4 changes: 2 additions & 2 deletions cli/run/watch-cli.ts
Expand Up @@ -4,11 +4,11 @@ import dateTime from 'date-time';
import ms from 'pretty-ms';
import onExit from 'signal-exit';
import * as rollup from '../../src/node-entry';
import { MergedRollupOptions, RollupWatcher } from '../../src/rollup/types';
import type { MergedRollupOptions, RollupWatcher } from '../../src/rollup/types';
import { bold, cyan, green, underline } from '../../src/utils/colors';
import relativeId from '../../src/utils/relativeId';
import { handleError, stderr } from '../logging';
import { BatchWarnings } from './batchWarnings';
import type { BatchWarnings } from './batchWarnings';
import { getConfigPath } from './getConfigPath';
import loadAndParseConfigFile from './loadConfigFile';
import loadConfigFromCommand from './loadConfigFromCommand';
Expand Down
18 changes: 9 additions & 9 deletions src/Bundle.ts
@@ -1,8 +1,8 @@
import Chunk from './Chunk';
import ExternalModule from './ExternalModule';
import Graph from './Graph';
import type ExternalModule from './ExternalModule';
import type Graph from './Graph';
import Module from './Module';
import {
import type {
GetManualChunk,
NormalizedInputOptions,
NormalizedOutputOptions,
Expand All @@ -13,8 +13,8 @@ import {
WarningHandler
} from './rollup/types';
import { FILE_PLACEHOLDER } from './utils/FileEmitter';
import { PluginDriver } from './utils/PluginDriver';
import { Addons, createAddons } from './utils/addons';
import type { PluginDriver } from './utils/PluginDriver';
import { type Addons, createAddons } from './utils/addons';
import { getChunkAssignments } from './utils/chunkAssignment';
import commondir from './utils/commondir';
import {
Expand All @@ -25,7 +25,7 @@ import {
warnDeprecation
} from './utils/error';
import { sortByExecutionOrder } from './utils/executionOrder';
import { GenerateCodeSnippets, getGenerateCodeSnippets } from './utils/generateCodeSnippets';
import { type GenerateCodeSnippets, getGenerateCodeSnippets } from './utils/generateCodeSnippets';
import { basename, isAbsolute } from './utils/path';
import { timeEnd, timeStart } from './utils/timers';

Expand Down Expand Up @@ -104,7 +104,7 @@ export default class Bundle {
}

private async addManualChunks(
manualChunks: Record<string, string[]>
manualChunks: Record<string, readonly string[]>
): Promise<Map<Module, string>> {
const manualChunkAliasByEntry = new Map<Module, string>();
const chunkEntries = await Promise.all(
Expand Down Expand Up @@ -305,7 +305,7 @@ function validateOptionsForMultiChunkOutput(
);
}

function getIncludedModules(modulesById: Map<string, Module | ExternalModule>): Module[] {
function getIncludedModules(modulesById: ReadonlyMap<string, Module | ExternalModule>): Module[] {
return [...modulesById.values()].filter(
(module): module is Module =>
module instanceof Module &&
Expand All @@ -317,7 +317,7 @@ function addModuleToManualChunk(
alias: string,
module: Module,
manualChunkAliasByEntry: Map<Module, string>
) {
): void {
const existingAlias = manualChunkAliasByEntry.get(module);
if (typeof existingAlias === 'string' && existingAlias !== alias) {
return error(errCannotAssignModuleToChunk(module.id, alias, existingAlias));
Expand Down
16 changes: 8 additions & 8 deletions src/Chunk.ts
Expand Up @@ -165,13 +165,13 @@ export default class Chunk {
private strictFacade = false;
private usedModules: Module[] = undefined as never;
constructor(
private readonly orderedModules: Module[],
private readonly orderedModules: readonly Module[],
private readonly inputOptions: NormalizedInputOptions,
private readonly outputOptions: NormalizedOutputOptions,
private readonly unsetOptions: Set<string>,
private readonly unsetOptions: ReadonlySet<string>,
private readonly pluginDriver: PluginDriver,
private readonly modulesById: Map<string, Module | ExternalModule>,
private readonly chunkByModule: Map<Module, Chunk>,
private readonly modulesById: ReadonlyMap<string, Module | ExternalModule>,
private readonly chunkByModule: ReadonlyMap<Module, Chunk>,
private readonly facadeChunkByModule: Map<Module, Chunk>,
private readonly includedNamespaces: Set<Module>,
private readonly manualChunkAlias: string | null
Expand Down Expand Up @@ -209,10 +209,10 @@ export default class Chunk {
private static generateFacade(
inputOptions: NormalizedInputOptions,
outputOptions: NormalizedOutputOptions,
unsetOptions: Set<string>,
unsetOptions: ReadonlySet<string>,
pluginDriver: PluginDriver,
modulesById: Map<string, Module | ExternalModule>,
chunkByModule: Map<Module, Chunk>,
modulesById: ReadonlyMap<string, Module | ExternalModule>,
chunkByModule: ReadonlyMap<Module, Chunk>,
facadeChunkByModule: Map<Module, Chunk>,
includedNamespaces: Set<Module>,
facadedModule: Module,
Expand Down Expand Up @@ -837,7 +837,7 @@ export default class Chunk {
}
}

private checkCircularDependencyImport(variable: Variable, importingModule: Module) {
private checkCircularDependencyImport(variable: Variable, importingModule: Module): void {
const variableModule = variable.module;
if (variableModule instanceof Module) {
const exportChunk = this.chunkByModule.get(variableModule);
Expand Down
40 changes: 23 additions & 17 deletions src/Module.ts
Expand Up @@ -735,32 +735,36 @@ export default class Module {
timeStart('analyse ast', 3);

this.astContext = {
addDynamicImport: this.addDynamicImport.bind(this),
addExport: this.addExport.bind(this),
addImport: this.addImport.bind(this),
addImportMeta: this.addImportMeta.bind(this),
addDynamicImport: (node: ImportExpression): void => this.addDynamicImport(node),
addExport: (
node: ExportAllDeclaration | ExportNamedDeclaration | ExportDefaultDeclaration
): void => this.addExport(node),
addImport: (node: ImportDeclaration): void => this.addImport(node),
addImportMeta: (node: MetaProperty): void => this.addImportMeta(node),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the advantage of doing this? Considering performance, my understanding is that .bind is now more efficient as it avoids the indirection of the additional function, and these functions are "hot" in that they are called very often during the tree-shaking phase. And TypeScript should be able to handle .bind correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that has nothing to do with typescript. there were perf issues with bind (in the past), I think those have been fixed tho.

it's just consistency, e.g.: https://github.com/rollup/rollup/pull/4361/files/437e90bc8e8e784d8b3667280d4890dbcfff9945#diff-bdf37a1325be24e5865c89e911c2f1125a1d857ec429afb790bde572bbe12b0fR761-R765

but I'll revert.

code, // Only needed for debugging
deoptimizationTracker: this.graph.deoptimizationTracker,
error: this.error.bind(this),
error: (props: RollupError, pos: number) => this.error(props, pos),
fileName, // Needed for warnings
getExports: this.getExports.bind(this),
getExports: () => this.getExports(),
getModuleExecIndex: () => this.execIndex,
getModuleName: this.basename.bind(this),
getModuleName: () => this.basename(),
getNodeConstructor: (name: string) => nodeConstructors[name] || nodeConstructors.UnknownNode,
getReexports: this.getReexports.bind(this),
getReexports: () => this.getReexports(),
importDescriptions: this.importDescriptions,
includeAllExports: () => this.includeAllExports(true),
includeDynamicImport: this.includeDynamicImport.bind(this),
includeVariableInModule: this.includeVariableInModule.bind(this),
includeDynamicImport: (node: ImportExpression) => this.includeDynamicImport(node),
includeVariableInModule: (variable: Variable) => this.includeVariableInModule(variable),
magicString: this.magicString,
module: this,
moduleContext: this.context,
options: this.options,
requestTreeshakingPass: () => (this.graph.needsTreeshakingPass = true),
traceExport: this.getVariableForExportName.bind(this),
traceVariable: this.traceVariable.bind(this),
requestTreeshakingPass: () => {
this.graph.needsTreeshakingPass = true;
},
traceExport: (name: string) => this.getVariableForExportName(name),
traceVariable: (name: string) => this.traceVariable(name),
usesTopLevelAwait: false,
warn: this.warn.bind(this)
warn: (warning: RollupWarning, pos: number) => this.warn(warning, pos)
};

this.scope = new ModuleScope(this.graph.scope, this.astContext);
Expand Down Expand Up @@ -1004,12 +1008,14 @@ export default class Module {

private addRelevantSideEffectDependencies(
relevantDependencies: Set<Module | ExternalModule>,
necessaryDependencies: Set<Module | ExternalModule>,
alwaysCheckedDependencies: Set<Module | ExternalModule>
necessaryDependencies: ReadonlySet<Module | ExternalModule>,
alwaysCheckedDependencies: ReadonlySet<Module | ExternalModule>
): void {
const handledDependencies = new Set<Module | ExternalModule>();

const addSideEffectDependencies = (possibleDependencies: Set<Module | ExternalModule>) => {
const addSideEffectDependencies = (
possibleDependencies: ReadonlySet<Module | ExternalModule>
) => {
for (const dependency of possibleDependencies) {
if (handledDependencies.has(dependency)) {
continue;
Expand Down
23 changes: 13 additions & 10 deletions src/ModuleLoader.ts
Expand Up @@ -549,12 +549,17 @@ export class ModuleLoader {
};
}

private async handleExistingModule(module: Module, isEntry: boolean, isPreload: PreloadType) {
private async handleExistingModule(
module: Module,
isEntry: boolean,
isPreload: PreloadType
): Promise<void> {
const loadPromise = this.moduleLoadPromises.get(module)!;
if (isPreload) {
return isPreload === RESOLVE_DEPENDENCIES
await (isPreload === RESOLVE_DEPENDENCIES
? waitForDependencyResolution(loadPromise)
: loadPromise;
: loadPromise);
return;
}
if (isEntry) {
module.info.isEntry = true;
Expand All @@ -564,7 +569,7 @@ export class ModuleLoader {
}
module.implicitlyLoadedAfter.clear();
}
return this.fetchModuleDependencies(module, ...(await loadPromise));
await this.fetchModuleDependencies(module, ...(await loadPromise));
}

private handleResolveId(
Expand All @@ -584,10 +589,8 @@ export class ModuleLoader {
moduleSideEffects: this.hasModuleSideEffects(source, true),
syntheticNamedExports: false
};
} else {
if (resolvedId.external && resolvedId.syntheticNamedExports) {
this.options.onwarn(errExternalSyntheticExports(source, importer));
}
} else if (resolvedId.external && resolvedId.syntheticNamedExports) {
this.options.onwarn(errExternalSyntheticExports(source, importer));
}
return resolvedId;
}
Expand Down Expand Up @@ -715,7 +718,7 @@ function isNotAbsoluteExternal(
);
}

async function waitForDependencyResolution(loadPromise: LoadModulePromise) {
async function waitForDependencyResolution(loadPromise: LoadModulePromise): Promise<void> {
const [resolveStaticDependencyPromises, resolveDynamicImportPromises] = await loadPromise;
return Promise.all([...resolveStaticDependencyPromises, ...resolveDynamicImportPromises]);
await Promise.all([...resolveStaticDependencyPromises, ...resolveDynamicImportPromises]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid the unnecessary "await as last statement", one could also just type this as Promise<unknown>. My understanding is that this avoids one implicit layer of Promise unwrapping the compiler has to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably so (albeit probably microscopic in the grand schema of things). I believe I saw this pattern across the code base. I guess there is no "good" way to return undefined if the caller ignores the function result entirely. I think it makes the code easier to read and also interpret the intend. if the only caller of a function which returns a result ignores the result it feels more like an oversight than done on purpose. that's all.

I'll revert.

}
8 changes: 4 additions & 4 deletions src/ast/scopes/ChildScope.ts
Expand Up @@ -51,8 +51,8 @@ export default class ChildScope extends Scope {
addUsedOutsideNames(
usedNames: Set<string>,
format: InternalModuleFormat,
exportNamesByVariable: ReadonlyMap<Variable, string[]>,
accessedGlobalsByScope: ReadonlyMap<ChildScope, Set<string>>
exportNamesByVariable: ReadonlyMap<Variable, readonly string[]>,
accessedGlobalsByScope: ReadonlyMap<ChildScope, ReadonlySet<string>>
): void {
for (const variable of this.accessedOutsideVariables.values()) {
if (variable.included) {
Expand All @@ -76,8 +76,8 @@ export default class ChildScope extends Scope {

deconflict(
format: InternalModuleFormat,
exportNamesByVariable: ReadonlyMap<Variable, string[]>,
accessedGlobalsByScope: ReadonlyMap<ChildScope, Set<string>>
exportNamesByVariable: ReadonlyMap<Variable, readonly string[]>,
accessedGlobalsByScope: ReadonlyMap<ChildScope, ReadonlySet<string>>
): void {
const usedNames = new Set<string>();
this.addUsedOutsideNames(usedNames, format, exportNamesByVariable, accessedGlobalsByScope);
Expand Down
4 changes: 2 additions & 2 deletions src/ast/scopes/ModuleScope.ts
Expand Up @@ -33,8 +33,8 @@ export default class ModuleScope extends ChildScope {

deconflict(
format: InternalModuleFormat,
exportNamesByVariable: ReadonlyMap<Variable, string[]>,
accessedGlobalsByScope: ReadonlyMap<ChildScope, Set<string>>
exportNamesByVariable: ReadonlyMap<Variable, readonly string[]>,
accessedGlobalsByScope: ReadonlyMap<ChildScope, ReadonlySet<string>>
): void {
// all module level variables are already deconflicted when deconflicting the chunk
for (const scope of this.children)
Expand Down