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

V2: Typescript Typechecking #3232

Merged
merged 20 commits into from
Jul 31, 2019
Merged
Show file tree
Hide file tree
Changes from 19 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
4 changes: 4 additions & 0 deletions packages/core/core/src/AssetGraphBuilder.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ export default class AssetGraphBuilder extends EventEmitter {
return {assetGraph: this.assetGraph, changedAssets: changedAssets};
}

async validate(): Promise<void> {
return this.requestGraph.completeValidations();
}

handleNodeAddedToAssetGraph(node: AssetGraphNode) {
switch (node.type) {
case 'dependency':
Expand Down
3 changes: 3 additions & 0 deletions packages/core/core/src/ConfigLoader.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ export default class ConfigLoader {
case 'transformation':
devDeps = parcelConfig.getTransformerNames(filePath);
break;
case 'validation':
devDeps = parcelConfig.getValidatorNames(filePath);
break;
case 'dependency':
devDeps = parcelConfig.getResolverNames();
break;
Expand Down
2 changes: 2 additions & 0 deletions packages/core/core/src/Parcel.js
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,8 @@ export default class Parcel {
};
this.#reporterRunner.report(event);

await this.#assetGraphBuilder.validate();

return event;
} catch (e) {
if (e instanceof BuildAbortError) {
Expand Down
17 changes: 16 additions & 1 deletion packages/core/core/src/ParcelConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ import type {
PackageName,
Packager,
Optimizer,
Reporter
Reporter,
Validator
} from '@parcel/types';
import {isMatch} from 'micromatch';
import {basename} from 'path';
Expand All @@ -29,6 +30,7 @@ export default class ParcelConfig {
namers: Pipeline;
runtimes: {[EnvironmentContext]: Pipeline};
packagers: GlobMap<PackageName>;
validators: GlobMap<Pipeline>;
optimizers: GlobMap<Pipeline>;
reporters: Pipeline;
pluginCache: Map<PackageName, any>;
Expand All @@ -43,6 +45,7 @@ export default class ParcelConfig {
this.packagers = config.packagers || {};
this.optimizers = config.optimizers || {};
this.reporters = config.reporters || [];
this.validators = config.validators || {};
this.pluginCache = new Map();
}

Expand All @@ -55,6 +58,7 @@ export default class ParcelConfig {
filePath: this.filePath,
resolvers: this.resolvers,
transforms: this.transforms,
validators: this.validators,
runtimes: this.runtimes,
bundler: this.bundler,
namers: this.namers,
Expand Down Expand Up @@ -91,6 +95,13 @@ export default class ParcelConfig {
return this.loadPlugins(this.getResolverNames());
}

getValidatorNames(filePath: FilePath): Array<string> {
let validators: Pipeline =
this.matchGlobMapPipelines(filePath, this.validators) || [];

return validators;
}

getTransformerNames(filePath: FilePath): Array<string> {
let transformers: Pipeline | null = this.matchGlobMapPipelines(
filePath,
Expand All @@ -103,6 +114,10 @@ export default class ParcelConfig {
return transformers;
}

async getValidators(filePath: FilePath): Promise<Array<Validator>> {
return this.loadPlugins(this.getValidatorNames(filePath));
}

async getTransformers(filePath: FilePath): Promise<Array<Transformer>> {
return this.loadPlugins(this.getTransformerNames(filePath));
}
Expand Down
31 changes: 30 additions & 1 deletion packages/core/core/src/RequestGraph.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ import type {
RequestGraphNode,
RequestNode,
SubRequestNode,
TransformationOpts
TransformationOpts,
ValidationOpts
} from './types';

type RequestGraphOpts = {|
Expand Down Expand Up @@ -97,12 +98,14 @@ export default class RequestGraph extends Graph<RequestGraphNode> {
assets: Array<InternalAsset>,
configRequests: Array<ConfigRequest>
}>;
runValidate: ValidationOpts => Promise<void>;
loadConfigHandle: () => Promise<Config>;
resolverRunner: ResolverRunner;
configLoader: ConfigLoader;
onAssetRequestComplete: (AssetRequestNode, Array<InternalAsset>) => mixed;
onDepPathRequestComplete: (DepPathRequestNode, AssetRequest | null) => mixed;
queue: PromiseQueue;
validationQueue: PromiseQueue;
farm: WorkerFarm;
config: ParcelConfig;
options: ParcelOptions;
Expand Down Expand Up @@ -135,6 +138,7 @@ export default class RequestGraph extends Graph<RequestGraphNode> {
}: RequestGraphOpts) {
this.options = options;
this.queue = new PromiseQueue();
this.validationQueue = new PromiseQueue();
this.onAssetRequestComplete = onAssetRequestComplete;
this.onDepPathRequestComplete = onDepPathRequestComplete;
this.config = config;
Expand All @@ -152,11 +156,20 @@ export default class RequestGraph extends Graph<RequestGraphNode> {
// AssetGraphBuilder, which avoids needing to pass the options through here.
this.farm = await WorkerFarm.getShared();
this.runTransform = this.farm.createHandle('runTransform');
this.runValidate = this.farm.createHandle('runValidate');
this.loadConfigHandle = WorkerFarm.createReverseHandle(
this.loadConfig.bind(this)
);
}

async completeValidations() {
if (!this.farm) {
await this.initFarm();
}

await this.validationQueue.run();
}

async completeRequests() {
if (!this.farm) {
await this.initFarm();
Expand Down Expand Up @@ -215,6 +228,9 @@ export default class RequestGraph extends Graph<RequestGraphNode> {
return result;
})
);

this.validationQueue.add(() => this.validate(requestNode));

break;
case 'dep_path_request':
promise = this.queue.add(() =>
Expand Down Expand Up @@ -249,6 +265,19 @@ export default class RequestGraph extends Graph<RequestGraphNode> {
}
}

async validate(requestNode: AssetRequestNode) {
try {
await this.runValidate({
request: requestNode.value,
loadConfig: this.loadConfigHandle,
parentNodeId: requestNode.id,
Copy link
Contributor

Choose a reason for hiding this comment

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

So the parentNodeId is provided so the config requests can attach themselves to the asset request node, so that if any of the config requests are invalidated, the asset request will be invalidated. Seems like if the typescript config changes, it would trigger the transformation to run again. Something like typescript config changing might warrant the transformation running again, but not something like eslintrc. Not sure if it's a huge issue because if the relevant config for a transformation doesn't change we'll just pick up from the cache instead of running the transformers. Would be better if we could figure out a way to not trigger all of that though.

Copy link
Member Author

@DeMoorJasper DeMoorJasper Jul 27, 2019

Choose a reason for hiding this comment

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

Not sure I completely understand? Currently validation isn't really cacheable and should run in each subsequent build no matter if there is cache available. Unless we'd figure out a way to return the errors/warnings instead of letting the validator throw them?

I just copied the request format from transforms, wasn't sure what most of it was used for but it's all being used somewhere afaik

Copy link
Contributor

Choose a reason for hiding this comment

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

The loadConfig function is passed in from the main process so that the results can be stored in the graph for both a faster retrieval when asking for that same config again and also so that the transformation can be invalidated when the config is invalidated. I was thinking that validator plugins would be using loadConfig as well since it was being passed in, but it looks like it is only being used for loading parcel config right now. If validator plugins were using it the way it is now, then a change in their config would invalidate the transformation causing it to run again. The configs actually used in the transformation wouldn't change though, so it would just end up using what it found in the cache.

Ideally validator and other plugins besides transformers should be using loadConfig, so that we can avoid rework of resolving configs between processes and so that we don't have to quit the process when we change configs. It would be a little weird if you only had to restart parcel when you change certain types of configs.

How I would see this working for validation would probably be something like the RequestGraph having an additional node type for a validation_request. It would be invalidated if the original file changes, the validator plugins change, or any config the plugins used change. This way changes to transformer plugins/config wouldn't trigger validations and vice versa.

options: this.options
});
} catch (e) {
throw e;
}
}

async transform(requestNode: AssetRequestNode) {
try {
let start = Date.now();
Expand Down
53 changes: 2 additions & 51 deletions packages/core/core/src/Transformation.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import nullthrows from 'nullthrows';
import type {
MutableAsset as IMutableAsset,
Blob,
FilePath,
GenerateOutput,
Transformer,
Expand All @@ -11,16 +10,10 @@ import type {
ParcelOptions,
PackageName
} from '@parcel/types';
import type {FileSystem} from '@parcel/fs';

import invariant from 'assert';
import path from 'path';
import {
md5FromReadableStream,
md5FromString,
md5FromObject,
TapStream
} from '@parcel/utils';
import {md5FromObject} from '@parcel/utils';
import Cache from '@parcel/cache';

import type Config from './public/Config';
Expand All @@ -30,15 +23,14 @@ import {report} from './ReporterRunner';
import {MutableAsset, assetToInternalAsset} from './public/Asset';
import InternalAsset from './Asset';
import type {NodeId, ConfigRequest} from './types';
import summarizeRequest from './summarizeRequest';

type GenerateFunc = (input: IMutableAsset) => Promise<GenerateOutput>;

type PostProcessFunc = (
Array<InternalAsset>
) => Promise<Array<InternalAsset> | null>;

const BUFFER_LIMIT = 5000000; // 5mb

export type TransformationOpts = {|
request: AssetRequest,
loadConfig: (ConfigRequest, NodeId) => Promise<Config>,
Expand Down Expand Up @@ -467,47 +459,6 @@ async function finalize(
return asset;
}

async function summarizeRequest(
fs: FileSystem,
req: AssetRequest
): Promise<{|content: Blob, hash: string, size: number|}> {
let code = req.code;
let content: Blob;
let hash: string;
let size: number;
if (code == null) {
// As an optimization for the common case of source code, while we read in
// data to compute its md5 and size, buffer its contents in memory.
// This avoids reading the data now, and then again during transformation.
// If it exceeds BUFFER_LIMIT, throw it out and replace it with a stream to
// lazily read it at a later point.
content = Buffer.from([]);
size = 0;
hash = await md5FromReadableStream(
fs.createReadStream(req.filePath).pipe(
new TapStream(buf => {
size += buf.length;
if (content instanceof Buffer) {
if (size > BUFFER_LIMIT) {
// if buffering this content would put this over BUFFER_LIMIT, replace
// it with a stream
content = fs.createReadStream(req.filePath);
} else {
content = Buffer.concat([content, buf]);
}
}
})
)
);
} else {
content = code;
hash = md5FromString(code);
size = Buffer.from(code).length;
}

return {content, hash, size};
}

function normalizeAssets(
results: Array<TransformerResult | MutableAsset>
): Array<TransformerResult> {
Expand Down
99 changes: 99 additions & 0 deletions packages/core/core/src/Validation.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
// @flow strict-local
import nullthrows from 'nullthrows';
import type {AssetRequest, ParcelOptions} from '@parcel/types';

import path from 'path';
import Cache from '@parcel/cache';
import {resolveConfig} from '@parcel/utils';

import type Config from './public/Config';
import {report} from './ReporterRunner';
import InternalAsset from './Asset';
import type {NodeId, ConfigRequest} from './types';
import {MutableAsset} from './public/Asset';
import summarizeRequest from './summarizeRequest';

export type ValidationOpts = {|
request: AssetRequest,
loadConfig: (ConfigRequest, NodeId) => Promise<Config>,
parentNodeId: NodeId,
options: ParcelOptions
|};

export default class Validation {
request: AssetRequest;
configRequests: Array<ConfigRequest>;
loadConfig: ConfigRequest => Promise<Config>;
options: ParcelOptions;
cache: Cache;
impactfulOptions: $Shape<ParcelOptions>;

constructor({request, loadConfig, parentNodeId, options}: ValidationOpts) {
this.request = request;
this.configRequests = [];
this.loadConfig = configRequest => {
this.configRequests.push(configRequest);
return loadConfig(configRequest, parentNodeId);
};
this.options = options;
}

async run(): Promise<void> {
if (this.request.filePath.includes('node_modules')) return;
Copy link
Member

Choose a reason for hiding this comment

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

Should we just not queue files in node_modules at all? Would avoid crossing the IPC boundary just to do nothing...

Copy link
Member Author

Choose a reason for hiding this comment

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

This was how it was originally but you commented on it to move it to the validatorrunner.

Will change it back. Well it wasn't a queue back then...

Copy link
Member

Choose a reason for hiding this comment

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

Oh.. haha sorry!

Copy link
Member

Choose a reason for hiding this comment

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

feel free to update in a followup


report({
type: 'validation',
request: this.request
});

this.cache = new Cache(this.options.outputFS, this.options.cacheDir);

let asset = await this.loadAsset();
let configRequest = {
filePath: this.request.filePath,
meta: {
actionType: 'validation'
}
};

let config = await this.loadConfig(configRequest);
let parcelConfig = nullthrows(config.result);

let validators = await parcelConfig.getValidators(this.request.filePath);
for (let validator of validators) {
await validator.validate({
asset: new MutableAsset(asset),
Copy link
Member

Choose a reason for hiding this comment

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

probably shouldn't be mutable for validators

Copy link
Member Author

Choose a reason for hiding this comment

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

Ow right, this was just copy-paste will change it to Asset

options: this.options,
resolveConfig: (configNames: Array<string>) =>
resolveConfig(this.options.inputFS, asset.filePath, configNames)
});
}
}

async loadAsset(): Promise<InternalAsset> {
let {filePath, env, code, sideEffects} = this.request;
let {content, size, hash} = await summarizeRequest(
this.options.inputFS,
this.request
);

return new InternalAsset({
// If the transformer request passed code rather than a filename,
// use a hash as the base for the id to ensure it is unique.
idBase: code != null ? hash : filePath,
fs: this.options.inputFS,
filePath: filePath,
type: path.extname(filePath).slice(1),
cache: this.cache,
ast: null,
content,
hash,
env: env,
stats: {
time: 0,
size
},
sideEffects: sideEffects
});
}
}
8 changes: 8 additions & 0 deletions packages/core/core/src/loadParcelConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,13 @@ export function validateConfigFile(
'transforms',
relativePath
);
validateMap(
config.validators,
validatePipeline.bind(this),
'validator',
'validators',
relativePath
);
validatePackageName(config.bundler, 'bundler', 'bundler', relativePath);
validatePipeline(config.namers, 'namer', 'namers', relativePath);
validateMap(
Expand Down Expand Up @@ -261,6 +268,7 @@ export function mergeConfigs(
filePath: ext.filePath, // TODO: revisit this - it should resolve plugins based on the actual config they are defined in
resolvers: mergePipelines(base.resolvers, ext.resolvers),
transforms: mergeMaps(base.transforms, ext.transforms, mergePipelines),
validators: mergeMaps(base.validators, ext.validators, mergePipelines),
bundler: ext.bundler || base.bundler,
namers: mergePipelines(base.namers, ext.namers),
runtimes: mergeMaps(base.runtimes, ext.runtimes),
Expand Down
Loading