enhance: upgrade local ts

[huozhi]

Fixes #606

• bump ts-loader version
• bump built-in ts version
• override incremental option to false

[styfle]

Thanks! Looks like CI is failing though.

[huozhi]

The inproper ts version in test directory lead to test failures

@styfle @guybedford, if I want to upgrade the ts lib assets generated in test/integration/node_modules/typescript/, (introduced #252) what's the proper coommand I should run? I tried ./dist/ncc/cli.js build src/typescript.js -o test/integration/node_modules/typescript/ -m but it emits unexpected assets

[guybedford]

@huozhi that sounds right to me, but TypeScript is a very complex build for ncc already - just updating TypeScript is a job unto itself. Is it working?

[styfle]

I'm not sure if a symlink for test/integration/node_modules/typescript/index.js is correct here. What do you think @guybedford ?

[guybedford]

@styfle the symlink approach seems fine to me actually. Note we should be sure to thoroughly test the TypeScript upgrade here manually before releasing, and it is a major release to upgrade TypeScript.

[styfle]

@guybedford All the CI tests are passing. Any other TS tests you have in mind?

[guybedford]

@styfle we may be missing test coverage of the bundled typescript version. That is, we should add a test that does a similar symlink trick, but to ./dist/typescript instead. Because otherwise node_modules/typescript will always be used as a local TypeScript always takes precedence so does in these tests as well.

[huozhi]

this ts upgration is truly a big gap (from 3.2.2 to 3.9.9).
one idea is to only upgrade to 3.4 to support incremental option, the gap is small.
another option is to change purpose of this PR to "upgrade local provided ts" and release later.

which one do you prefer?

[codecov-io]

Codecov Report

Merging #674 (9678c2b) into master (630d44e) will decrease coverage by 1.51%.
The diff coverage is 77.93%.

@@            Coverage Diff             @@
##           master     #674      +/-   ##
==========================================
- Coverage   74.93%   73.41%   -1.52%     
==========================================
  Files          13       13              
  Lines         399      474      +75     
==========================================
+ Hits          299      348      +49     
- Misses        100      126      +26     

Impacted Files	Coverage Δ
src/@@notfound.js	0.00% <0.00%> (ø)
src/typescript.js	80.00% <75.00%> (+2.22%)	⬆️
src/index.js	79.32% <76.98%> (-4.61%)	⬇️
src/cli.js	62.85% <91.66%> (+0.50%)	⬆️
src/loaders/empty-loader.js	100.00% <100.00%> (ø)
src/loaders/relocate-loader.js	100.00% <100.00%> (ø)
src/utils/get-package-base.js	0.00% <0.00%> (-37.50%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57bdfc5...9678c2b. Read the comment docs.

[styfle]

Looks good! Just missing this: #674 (comment)

Also in regard to the TS version, we bundle TS so its not going to use any version besides the one defined in devDependencies (3.6.5 in your PR or rather the version in the lock file)

[huozhi]

I'm kinda unclear about the approach of testing ./dist/typescript.js with symlink, would you mind clarifying it a bit more specific? I can work on that then. Thanks

[guybedford]

@huozhi perhaps a symlink is the wrong direction actually - instead we just want to not load the local test/node_modules/typescript by ensuring that TypeScript cannot be located.

One way to do this would be to construct a tmp folder at the system root - /tmp/s9d8f0s9d8/test.ts and then execute that ts file. Because there is no node_modules containing typescript from such a path, this will force ncc to use its local inbuilt typescript version and thus test that.

Alternatively, simply running ncc from a difference cwd could also do this:

cd /
ncc run /path/to/ncc/test/unit/ts-ext/test.ts

Tests like the above might be possible to construct with the test/cli.js file (https://github.com/vercel/ncc/blob/master/test/cli.js) by adding a cwd option support to that test system.

So that's two possible ways to test this, basically if ncc finds a node_modules/typescript in cwd, it uses it, and notes that it is in the console. And because ncc/node_modules and ncc/test/node_modules both have local typescript versions, we are never testing the case where it isn't found. If you can think of another way apart from my above two suggestions to catch this other case feel free to try something else too and just let me know if I can clarify further at all.

[huozhi]

I use the TYPESCRIPT_LOOKUP_PATH env as override option for tests. If the folder is invalid ncc can fallback to built-in ts lib. hope it's sth you're expecting.

[guybedford]

Looks great, thanks for your work on this!

[styfle]

Great work, thanks! 🎉