V2: Typescript Typechecking #3232
Conversation
There was a problem hiding this comment.
Looking good! 🎉
I haven't actually run it yet, but I'm curious what the terminal output (in the reporter) looks like for this? I imagine the build succeeds, and it prints the bundle report, but then later it "fails" with some validation errors? Seems like it might be confusing... And what about in watch/serve mode? Should they be different?
packages/core/core/src/Validation.js
Outdated
| let configRequest = { | ||
| filePath, | ||
| meta: { | ||
| actionType: 'validation' |
There was a problem hiding this comment.
Does this actionType need to be implemented in the ConfigLoader?
There was a problem hiding this comment.
I guess so, don't completely understand how the config works at the moment
There was a problem hiding this comment.
cc. @padmaia I think it needs to return the names of the validator plugins maybe? https://github.com/parcel-bundler/parcel/blob/v2/packages/core/core/src/ConfigLoader.js#L42-L49
There was a problem hiding this comment.
I updated it to return names of validators as it's like that for transforms.
@padmaia is this how it's supposed to be?
packages/core/core/src/Validation.js
Outdated
| await validator.validate({ | ||
| asset: new MutableAsset(asset), | ||
| options: this.options, | ||
| resolve |
There was a problem hiding this comment.
I think resolve probably isn't needed for validators? Could add later if needed.
There was a problem hiding this comment.
Not sure, Would be nice to overwrite the TS resolver with whatever resolver Parcel uses. Although not sure if it's a good idea because that might give unexpected behaviour compared to running tsc
There was a problem hiding this comment.
Removed the resolve function for now
|
|
||
| let configNames = ['tsconfig.json']; | ||
| // $FlowFixMe | ||
| let configPath = await resolveConfig(asset.filePath, configNames); |
There was a problem hiding this comment.
todo: another plugin type to add a loadConfig method to. cc. @padmaia
| await this.runValidate({ | ||
| request: requestNode.value, | ||
| loadConfig: this.loadConfigHandle, | ||
| parentNodeId: requestNode.id, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
packages/core/core/src/Validation.js
Outdated
| let validators = await parcelConfig.getValidators(this.request.filePath); | ||
| for (let validator of validators) { | ||
| await validator.validate({ | ||
| asset: new MutableAsset(asset), |
There was a problem hiding this comment.
probably shouldn't be mutable for validators
There was a problem hiding this comment.
Ow right, this was just copy-paste will change it to Asset
| } | ||
|
|
||
| async run(): Promise<void> { | ||
| if (this.request.filePath.includes('node_modules')) return; |
There was a problem hiding this comment.
Should we just not queue files in node_modules at all? Would avoid crossing the IPC boundary just to do nothing...
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
feel free to update in a followup
| export type Validator = {| | ||
| validate({ | ||
| asset: MutableAsset, | ||
| resolveConfig: ResolveConfigFn, // This is a temporary function and should be replaced with something cacheable |
There was a problem hiding this comment.
sounds like a good followup task
|
Everything should be updated now. Ready for another review. Will do config things in a follow-up PR |
↪️ Pull Request
💻 Examples
🚨 Test instructions
✔️ PR Todo