Add typechecking to default parcelconfig #4025

DeMoorJasper · 2020-01-21T18:33:42Z

↪️ Pull Request

This PR enabled TS typechecking by default. This is probably more expected behaviour than the current silent behaviour.

Related #4022

✔️ PR Todo

Added/updated unit tests for this change
Filled out test instructions (In case there aren't any unit tests)
Included links to related issues/PRs

parcel-benchmark · 2020-01-21T19:05:51Z

Benchmark Results

packages/benchmarks/kitchen-sink ✅

Timings

Description	Time	Difference
Cold	7.75s	+107.00ms
Cached	3.90s	+13.00ms

Cold Bundles

Bundle	Size	Difference	Time	Difference
dist/legacy/parcel.ff215454.webp	102.94kb	+0.00b	233.00ms	-16.00ms 🚀

Cached Bundles

Bundle	Size	Difference	Time	Difference
dist/legacy/parcel.ff215454.webp	102.94kb	+0.00b	332.00ms	-26.00ms 🚀
dist/modern/parcel.76ee4591.webp	102.94kb	+0.00b	330.00ms	-27.00ms 🚀
dist/legacy/index.js	520.00b	+0.00b	336.00ms	-27.00ms 🚀
dist/modern/index.js	453.00b	+0.00b	331.00ms	-26.00ms 🚀
dist/legacy/index.css	29.00b	+0.00b	330.00ms	-27.00ms 🚀
dist/modern/index.css	29.00b	+0.00b	329.00ms	-26.00ms 🚀

packages/benchmarks/react-hn ✅

Timings

Description	Time	Difference
Cold	11.79s	+649.00ms ⚠️
Cached	3.88s	+33.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

Bundle	Size	Difference	Time	Difference
dist/module/index.js	34.29kb	+0.00b	12.00ms	+1.00ms ⚠️
dist/main/index.js	34.27kb	+0.00b	17.00ms	+1.00ms ⚠️

packages/benchmarks/ak-editor ✅

Timings

Description	Time	Difference
Cold	3.32m	+3.97s
Cached	5.99s	-109.00ms

Cold Bundles

Bundle	Size	Difference	Time	Difference
dist/index.js	2.33mb	+6.00b ⚠️	1.93m	+2.79s
dist/Toolbar.5c7a6fbb.js	105.61kb	+0.00b	1.30m	+15.44s ⚠️
dist/media-viewer.81fbeba4.js	87.54kb	+0.00b	7.00s	-2.00s 🚀
dist/card.c0473744.js	76.79kb	+0.00b	7.00s	-2.00s 🚀
dist/smartMediaEditor.25b0ed57.js	67.35kb	+0.00b	58.59s	-4.08s 🚀
dist/ResourcedEmojiComponent.2d80adbd.js	2.25kb	+0.00b	6.94s	-2.00s 🚀
dist/esm.976debcc.js	178.00b	+0.00b	6.86s	-1.98s 🚀
dist/dropzone.976debcc.js	177.00b	+1.00b ⚠️	1.93m	+1.30m ⚠️
dist/clipboard.976debcc.js	177.00b	+1.00b ⚠️	37.32s	-804.00ms
dist/browser.976debcc.js	177.00b	+1.00b ⚠️	58.59s	+20.47s ⚠️
dist/editorView.976debcc.js	177.00b	-1.00b 🚀	36.78s	-25.89s 🚀
dist/media-card-analytics-error-boundary.976debcc.js	176.00b	-5.00b 🚀	38.68s	-1.24m 🚀
dist/16.976debcc.js	174.00b	-5.00b 🚀	37.32s	-2.09s 🚀

Cached Bundles

Bundle	Size	Difference	Time	Difference
dist/index.js	2.33mb	+6.00b ⚠️	411.00ms	+35.00ms ⚠️
dist/esm.976debcc.js	176.00b	+2.00b ⚠️	160.00ms	-0.00ms
dist/media-card-analytics-error-boundary.976debcc.js	176.00b	+2.00b ⚠️	159.00ms	+1.00ms
dist/dropzone.976debcc.js	176.00b	+2.00b ⚠️	156.00ms	+1.00ms
dist/clipboard.976debcc.js	176.00b	+2.00b ⚠️	155.00ms	+1.00ms
dist/browser.976debcc.js	176.00b	+2.00b ⚠️	155.00ms	+1.00ms
dist/16.976debcc.js	176.00b	+2.00b ⚠️	145.00ms	-0.00ms
dist/editorView.976debcc.js	176.00b	+2.00b ⚠️	130.00ms	-0.00ms

Click here to view a detailed benchmark overview.

DeMoorJasper · 2020-02-22T10:57:51Z

Gonna close this...

uglycoyote · 2020-04-17T22:57:42Z

@DeMoorJasper it's not clear from looking at this why it was closed?

DeMoorJasper · 2020-04-18T07:18:07Z

@uglycoyote it had gotten too much merge conflicts and I doubt it would've ever gotten merged. From the issue it seems there's almost just as much people who want this as don't want this

astegmaier · 2020-04-18T16:05:33Z

@DeMoorJasper - I think you're referring to #4022, right? My reading of that (very long, very contentious) thread was that there were at least two issues being discussed:

Whether to use tsc instead of babel to do the actual transpilation
Having parcel check for typescript semantic errors by default (i.e. this PR)

It seemed like a lot of folks who were new to parcel didn't understand this distinction at first (until this comment helped clarify toward the end of the discussion).

It seems accurate to say that issue 1 was very contentious because there are some very solid performance-related reasons to prefer babel. For issue number 2, it seemed more one-sided in favor of turning on semantic checking by default (although not 100% unanimous). I'd love to see a wider discussion around this comment, which posed the question directly.

My two-cents as a heavy typescript user is that I pretty much always want to get semantic type error feedback from my build tool, so it would be great to have this be the default. This is for two main reasons:

I come from a background of having used tsc, webpack (with fork-ts-webpack-checker-plugin and ts-loader) and create-react-app on various projects, all of which do this. (This is more of a habit/subjective expectation, but I suspect most of parcel's typescript users will share a similar background).
As this comment points out, VSCode and other editors won't report errors on files that are not open, and don't seem likely to support this in the future.

DeMoorJasper · 2020-04-18T16:12:38Z

@astegmaier You’re right opinions were very mixed. I don’t mind typechecking being the default

The main reason for closing this was merge conflicts as I have enough PRs that need rebasing and have a higher chance of getting merged soonish

astegmaier · 2020-04-18T16:22:32Z

The main reason for closing this was merge conflicts as I have enough PRs that need rebasing and have a higher chance of getting merged soonish

Yeah no worries. If the discussion develops and we get everyone on board with the idea of turning this on by default, I'm happy to take care of the PR.

uglycoyote · 2020-04-19T01:40:40Z

Thanks for the replies. I didn't mean to cause a firestorm and I know there's a bit of a debate about this contentious issue on #4022, which I have replied on a few times. But I was curious because the effort to change the default behaviour was started by someone who's a primary contributor and then abruptly stopped without an explanation.

Upon reading this, since the "gonna close this" comment was one day after posting the benchmarks, It made me wonder if the performance numbers in the benchmarks are what clinched the decision to close it, but it sound like it was other issues.

I looked at the benchmarks but there were too many numbers and I didn't know what the format of the report was, I could not really tell whether they said something interesting about the performance implications of this change. I'm aware that performance is the big argument in favour of leaving it the way it was (with no use of typechecking) but had not seen any figures about what the impact of enabling typechecking was by default.

astegmaier · 2020-04-19T04:22:20Z

Correct me if I'm wrong, but I don't think turning on @parcel/validator-typescript by default (what I called "issue 2" above) should impact the performance of the main build, because validators only run after the bundle graph is finished. See here:

parcel/packages/core/core/src/Parcel.js

Lines 268 to 284 in 7c0aa3d

    
           await this.#packagerRunner.writeBundles(bundleGraph); 
        
           assertSignalNotAborted(signal); 
        
           let event = { 
        
             type: 'buildSuccess', 
        
             changedAssets: new Map( 
        
               Array.from(changedAssets).map(([id, asset]) => [ 
        
                 id, 
        
                 assetFromValue(asset, options), 
        
               ]), 
        
             ), 
        
             bundleGraph: new BundleGraph(bundleGraph, options), 
        
             buildTime: Date.now() - startTime, 
        
           }; 
        
           this.#reporterRunner.report(event); 
        
           await this.#assetGraphBuilder.validate();

(It's a different story for using @parcel/transformer-typescript-tsc` over babel during the transform phase - see this comment).

devongovett · 2020-04-19T05:58:20Z

Please continue this discussion on #4022 rather than on this closed PR.

enable typechecking by default

f0627b9

DeMoorJasper mentioned this pull request Jan 21, 2020

Parcel 2: TypeScript automatic zero configuration not type checking or reading tsconfig.json #4022

Closed

Merge branch 'v2' into ts-checking-default

e6587ab

DeMoorJasper requested a review from devongovett January 23, 2020 15:44

mischnic added the ✨ Parcel 2 label Jan 28, 2020

DeMoorJasper closed this Feb 22, 2020

DeMoorJasper deleted the ts-checking-default branch February 22, 2020 10:57

astegmaier mentioned this pull request Feb 28, 2020

Parcel2: Better error messages when you re-export an implicit type. #4240

Open

parcel-bundler locked and limited conversation to collaborators Apr 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typechecking to default parcelconfig #4025

Add typechecking to default parcelconfig #4025

DeMoorJasper commented Jan 21, 2020

parcel-benchmark commented Jan 21, 2020 •

edited

Loading

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

DeMoorJasper commented Feb 22, 2020

uglycoyote commented Apr 17, 2020

DeMoorJasper commented Apr 18, 2020

astegmaier commented Apr 18, 2020

DeMoorJasper commented Apr 18, 2020 •

edited

Loading

astegmaier commented Apr 18, 2020

uglycoyote commented Apr 19, 2020 •

edited

Loading

astegmaier commented Apr 19, 2020

devongovett commented Apr 19, 2020

Add typechecking to default parcelconfig #4025

Add typechecking to default parcelconfig #4025

Conversation

DeMoorJasper commented Jan 21, 2020

↪️ Pull Request

✔️ PR Todo

parcel-benchmark commented Jan 21, 2020 • edited Loading

Benchmark Results

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

DeMoorJasper commented Feb 22, 2020

uglycoyote commented Apr 17, 2020

DeMoorJasper commented Apr 18, 2020

astegmaier commented Apr 18, 2020

DeMoorJasper commented Apr 18, 2020 • edited Loading

astegmaier commented Apr 18, 2020

uglycoyote commented Apr 19, 2020 • edited Loading

astegmaier commented Apr 19, 2020

devongovett commented Apr 19, 2020

parcel-benchmark commented Jan 21, 2020 •

edited

Loading

DeMoorJasper commented Apr 18, 2020 •

edited

Loading

uglycoyote commented Apr 19, 2020 •

edited

Loading