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 #3142

Closed
wants to merge 32 commits into from

Conversation

@DeMoorJasper
Copy link
Member

DeMoorJasper commented Jun 9, 2019

↪️ Pull Request

This PR implements the TypeScript TypeChecking Validator

✔️ PR Todo

  • Write validator plugin type
  • Extract typechecking from parcel-plugin-ts into a validator plugin
  • Add examples
@devongovett

This comment has been minimized.

Copy link
Member

devongovett commented Jun 9, 2019

This is very cool. I agree. We should add a new plugin type for this. Then we can run the typechecker in the background so that it doesn’t block the build in development. As you said, this will be useful for things like linters as well.

@DeMoorJasper

This comment has been minimized.

Copy link
Member Author

DeMoorJasper commented Jun 9, 2019

@devongovett awesome, about config though do you think it would be a good idea or even possible to return a path as well? I'm not entirely sure TypeScript needs it but it's how it's stated in TS docs, so I guess it's supposed to be that.

Also I'm not entirely sure how to properly emit errors so the reporter only reports it once. Parcel 1 used to have some props in error that made this simple, couldn't find this in the Parcel 2 types of logger

@devongovett

This comment has been minimized.

Copy link
Member

devongovett commented Jun 9, 2019

Not sure what you mean by return a path...

For the errors, can you somehow filter the errors in the checker to only throw type errors rather than other types of error (e.g. syntax errors)? Only type errors are not already handled by the transformer.

Some other questions:

  • What about resolving? How do we make sure the way the TS checker resolves and the way the TS transformer resolves are the same? I guess we'll be adding a TS resolver plugin, but we'll need the TS checker plugin to also use that.
  • From looking at the code, @fathyb's plugin does a lot more than what is here. I'm not totally sure what exactly, but do we need to carry any of that over?
@DeMoorJasper

This comment has been minimized.

Copy link
Member Author

DeMoorJasper commented Jun 9, 2019

As far as I've been able to look through fathyb's code, it does some resolving (mainly for transform) and some enhancements to improve performance of typechecking (as it's insanely slow, but we'll add a custom plugin type for this so we can optimise it in core).
I think it should be sufficient to get the path of the tsconfig it's using to let ts do all the resolving by itself relative to this config.

About resolving I think typescript should be able to handle it. It's using the langService which should support all the advanced TS resolving already as far as I understand. Once transformers support all tsconfig resolving things it should be identical.

Would be nice to have someone like @fathyb with more TS experience to look at this PR though to validate I didn't miss anything and give some more guidance

@devongovett

This comment has been minimized.

Copy link
Member

devongovett commented Jun 9, 2019

But parcel specific things like aliases won't work though right?

@DeMoorJasper

This comment has been minimized.

Copy link
Member Author

DeMoorJasper commented Jun 9, 2019

You need to configure those in tsconfig anyway, to get things like IDE integration working.

But you're right this won't work out of the box with Parcel things without configuring it in tsconfig

@devongovett

This comment has been minimized.

Copy link
Member

devongovett commented Jun 9, 2019

IDE integration would be problematic with other languages like plain JS though too, and we still support aliases there...

@devongovett

This comment has been minimized.

Copy link
Member

devongovett commented Jun 9, 2019

Seems like we could probably convince vs code to support aliases defined in package.json. Tilde paths may be more contentious.

@DeMoorJasper

This comment has been minimized.

Copy link
Member Author

DeMoorJasper commented Jun 9, 2019

That would be awesome.

In case of typescript all these things (aliasing, absolute tilde) can be configured in tsconfig.

However it would be a big timesaver if vscode would support these things out of the box without the need of extra config, especially helpful for JS as you mentioned

@DeMoorJasper

This comment has been minimized.

Copy link
Member Author

DeMoorJasper commented Jun 10, 2019

Should be ready for review

config: await asset.getConfig(['tsconfig.json']),
baseDir:
path.dirname(
await findUp('tsconfig.json', {

This comment has been minimized.

Copy link
@mischnic

mischnic Jun 10, 2019

Member

findUp returns undefined if it can't find a tsconfig. (and then path.dirname(undefined) throws)
And in general: what should happen if no tsconfig is provided? Fallback to a default config?

This comment has been minimized.

Copy link
@DeMoorJasper

DeMoorJasper Jun 10, 2019

Author Member

TypeScript already handles empty configs, but not sure if we should provide a custom default

This comment has been minimized.

Copy link
@DeMoorJasper

DeMoorJasper Jun 10, 2019

Author Member

I also think findUp should just be a temporary solution and there should be some util that's shared with config for resolving config paths.
Fixed the potential issue anyway

This comment has been minimized.

Copy link
@mischnic

mischnic Jun 10, 2019

Member

Well, the tests fail (even with that fix):
If config: await asset.getConfig(['tsconfig.json']) is null, typescript throws an error.

This comment has been minimized.

Copy link
@DeMoorJasper

DeMoorJasper Jun 10, 2019

Author Member

ah damn, will update it to default to {}

This comment has been minimized.

Copy link
@DeMoorJasper

DeMoorJasper Jun 10, 2019

Author Member

Should all be fixed now and no longer even use findUp...

@wbinnssmith wbinnssmith force-pushed the wbinnssmith/tsc branch from 510277d to 3203d69 Jun 10, 2019
@@ -150,6 +153,9 @@ export default class RequestGraph extends Graph<RequestGraphNode> {
async transform(request: AssetRequest) {
try {
let start = Date.now();
if (!(await realpath(request.filePath)).includes('node_modules')) {
await this.runValidate(request);

This comment has been minimized.

Copy link
@devongovett

devongovett Jun 11, 2019

Member

Do we actually need to await this here or could we run this in the background? For development mode, it might be nice to not block the build based on type checking.

This comment has been minimized.

Copy link
@DeMoorJasper

DeMoorJasper Jun 11, 2019

Author Member

depends, if it throws errors it should probably be awaited as we'd otherwise have uncaught promise rejections.

Although I think after the slack discussion it's safe to say we probably never wanna fail a build and just log warnings.

This comment has been minimized.

Copy link
@devongovett

devongovett Jun 16, 2019

Member

Maybe we could just queue the validations here and then run them after the build is complete?

This comment has been minimized.

Copy link
@DeMoorJasper

DeMoorJasper Jun 16, 2019

Author Member

Not sure what the benefit of that would be unless we'd spawn a dedicated worker to run all typechecking. as that'll allow ts to keep a state and not reparse configs, recheck files and whatnot

This comment has been minimized.

Copy link
@devongovett

devongovett Jun 17, 2019

Member

The benefit is faster builds in development. When changing code in development, we want to compile as quickly as possible and trigger HMR. TS can happen in the background after that. This means not using the workers for TS checking until after the build is complete.

This comment has been minimized.

Copy link
@devongovett

devongovett Jun 17, 2019

Member

The idea of keeping state is interesting too. We could have a dedicated "validator" worker to run TS and other linters in maybe... I think that's how fork-ts-checker-webpack-plugin works, and fathy's plugin looks like it has its own worker too...

This comment has been minimized.

Copy link
@DeMoorJasper

DeMoorJasper Jun 17, 2019

Author Member

Yes most plugins do it that way. I think it's the most performant way of doing this. Haven't experimented with running all workers with a TypeChecking state but I guess it wouldn't be much faster or even slower and just waste resources. I'll experiment with both once the TS PR is in and some SourceMap bugs are fixed.
This validator PR is not really high priority anyway and it was just a bit of an experiment to see if and how we'd support this.

@@ -150,6 +153,9 @@ export default class RequestGraph extends Graph<RequestGraphNode> {
async transform(request: AssetRequest) {
try {
let start = Date.now();
if (!(await realpath(request.filePath)).includes('node_modules')) {

This comment has been minimized.

Copy link
@devongovett

devongovett Jun 11, 2019

Member

Maybe do this check inside the validator runner?

This comment has been minimized.

Copy link
@DeMoorJasper

DeMoorJasper Jun 11, 2019

Author Member

Sure will update it


const BUFFER_LIMIT = 5000000; // 5mb

export default class validatorRunner {

This comment has been minimized.

Copy link
@devongovett

devongovett Jun 11, 2019

Member

hmm a bunch of stuff in here seems duplicated from transformers... wonder what we can reuse.

This comment has been minimized.

Copy link
@DeMoorJasper

DeMoorJasper Jun 11, 2019

Author Member

Most of it's copy paste from transformerrunner

@@ -0,0 +1,8762 @@
declare module 'typescript' {

This comment has been minimized.

Copy link
@wbinnssmith

wbinnssmith Jun 11, 2019

Member

where did this come from? doesn't have the usual flow-typed digest on top 🤔

This comment has been minimized.

Copy link
@wbinnssmith

wbinnssmith Jun 12, 2019

Member

Ah, we should probably be installing flow-typed definitions with flow-typed install typescript@3.3.x so it can manage them for us. I might do this over in my branch.

return;
}

const content = this.ts.sys.readFile(fileName);

This comment has been minimized.

Copy link
@devongovett

devongovett Jun 16, 2019

Member

Presumably we'll want to get this from the asset somehow, e.g. asset.getCode(). Otherwise, typescript e.g. inside a vue file or other virtual assets won't work. Actually, now that I think of it, not sure that would work anyway since validators as they are currently set up work on input files, not on intermediary assets... hmm

This comment has been minimized.

Copy link
@DeMoorJasper

DeMoorJasper Jun 16, 2019

Author Member

It’s more than just the current asset as it also does imports afaik so not sure how to do that.

This comment has been minimized.

Copy link
@devongovett

devongovett Jun 17, 2019

Member

Ah yeah ok

}

getCurrentDirectory() {
return process.cwd();

This comment has been minimized.

Copy link
@devongovett

devongovett Jun 16, 2019

Member

Should this be based on project root?

let tsconfig = (await asset.getConfig(configNames)) || {};
let baseDir = configPath ? path.dirname(configPath) : options.projectRoot;

let parsedCommandLine = ts.parseJsonConfigFileContent(

This comment has been minimized.

Copy link
@devongovett

devongovett Jun 16, 2019

Member

Will this result in re-parsing the config for every file?

This comment has been minimized.

Copy link
@DeMoorJasper

DeMoorJasper Jun 16, 2019

Author Member

Yes unfortunately it will

@DeMoorJasper DeMoorJasper changed the base branch from wbinnssmith/tsc to v2 Jun 26, 2019
@DeMoorJasper DeMoorJasper deleted the v2-typechecking branch Jul 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.