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

major: replace ts-loader with swc #926

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

kwonoj
Copy link

@kwonoj kwonoj commented May 11, 2022

Breaking Changes

  • No more type checking, run tsc instead
  • No more emit of .d.ts files, run tsc instead

Description

PR reattempt #777 with updated dependencies, as well as fixing few immediate code paths to pass existing tests. There are few tests removed, related with ts-error fixtures which is redundant now as swc doesn't run type check while compiling.

Some of fixtures has changed its output mostly due to differences between how swc / tsc emits transformed data. I read it manually and looks like it's non-breaking changes, but still need some eyes.

@styfle
Copy link
Member

styfle commented May 26, 2022

Thanks for the PR!

I've testing on real repos here:

This needs further investigation before we land since its not clear why the require() in TS behaves differently

@kwonoj
Copy link
Author

kwonoj commented May 26, 2022

its not clear why the require() in TS behaves differently

Agreed, somehow only on swc it try to replace it into bundled require logic. I may need further digging to find out reason.

@Timer Timer marked this pull request as draft June 15, 2022 04:11
@Timer Timer removed their request for review June 15, 2022 04:11
@styfle
Copy link
Member

styfle commented Jul 26, 2022

@kwonoj I found a bug where the .d.ts files are not emitted with ncc even thought the tsconfig.json has "declaration": true. You can see this had to be changed in vercel/vercel af779ab

@kwonoj
Copy link
Author

kwonoj commented Jul 26, 2022

even thought the tsconfig.json has "declaration": true

SWC does not have capability to emit type declaration, this would be (somewhat) expected behavior in this changes, since PR entirely replaces typescript into SWC.

Should we preserve type declaration emission feature? if that's the case, sounds like that's a hard blocker to push SWC for ncc.

@styfle
Copy link
Member

styfle commented Jul 27, 2022

@kwonoj Hmm, I think replacing tsloader with swc is a fair tradeoff to losing .d.ts since swc is 2x faster in vercel/vercel and vercel/next.js. In the cases we need .d.ts, we can run the slower tsc to generate.

Please update this PR to fix the conflicts and then we can merge, thanks!

@styfle styfle changed the title feat(loaders): replace tsloader into swc major(loaders): replace tsloader with swc Jul 27, 2022
@kwonoj kwonoj force-pushed the feat-swc branch 3 times, most recently from 2615a20 to dd6053b Compare July 27, 2022 17:51
@styfle styfle changed the title major(loaders): replace tsloader with swc major(loaders): replace ts-loader with swc Jul 27, 2022
@styfle styfle changed the title major(loaders): replace ts-loader with swc major: replace ts-loader with swc Jul 27, 2022
},
{
args: ["run", "-t", "test/fixtures/with-type-errors/ts-error.ts"],
expect: { code: 0 }
Copy link
Member

Choose a reason for hiding this comment

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

Why were these 3 tests removed?

Copy link
Author

Choose a reason for hiding this comment

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

These tests are specific to verify tsc to fail if given typescript file contains error: for example, test verifies output

code === 1 && stderr.toString().indexOf('ts-error.ts(3,16)') !== -1

to check tsc's error message contains error line to 3:16 where it have type-related error:

//3:16 points 'Y', since there's no corresponding types
function p (x: Y) { 

which doesn't have any meaningful behavior in swc as it strips out typescript and does not emit any type check failures.

Copy link
Member

Choose a reason for hiding this comment

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

Ok so to be clear, this PR introduces another breaking change: no more type checking?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it removes out of the box support from running ncc. If we are considering that is a mandatory feature we need to support, I'm afraid it's not achievable via swc.

Copy link
Member

Choose a reason for hiding this comment

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

Ok I update the PR title and description to mention this is semver major and has breaking changes.

Also, it seems there are still failing tests

Copy link
Author

Choose a reason for hiding this comment

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

Huh, weird that only subset of node.js 16 CI's failing. I'll try to peek.

@SukkaW
Copy link

SukkaW commented Jul 28, 2022

its not clear why the require() in TS behaves differently

Agreed, somehow only on swc it try to replace it into bundled require logic. I may need further digging to find out reason.

@kwonoj @styfle

FYI, the following code behaves differently in tsc and swc:

// a.js
console.log('a')

// c.js
console.log('c')

// main.js
import a from './a'
console.log('b')
import b from './c'

To be fully compliant with the ESM spec, main.js should yield a c b, but with tsc it will yield a b c.

I don't know if it is the case, so I will raise it here.

@kwonoj
Copy link
Author

kwonoj commented Jul 28, 2022

To be fully compliant with the ESM spec, main.js should yield a c b, but with tsc it will yield a b c.

This is true, but afaik the issue in here I observed was not this one.

@kwonoj
Copy link
Author

kwonoj commented Jul 28, 2022

@styfle Looks like CI failures are flaky, need additional investigation.

Currently PR passes all the jobs, but the last commit is dummy (fe3837b) which only increases timeout + console.out for stdout/err compare to previous commit failed on some jobs.

src/index.js Outdated Show resolved Hide resolved
@cb1kenobi
Copy link

cb1kenobi commented Jul 29, 2022

I'm not sure what's going on, but I'm seeing a huge performance regression. I have a super small test package with a src/index.ts (contains only 1 exported function) and src/types.ts (contains 3 tiny interfaces). My package is an ESM package: "type": "module".

The latest ncc v0.34.0 can consistently build my package in about 1.5-2 seconds. Command is ncc build src/index.ts --minify. I get type checking and type definition files.

➜  myncctest git:(master) ✗ rm -rf dist && time pnpm run build

> [email protected] build /home/chris/projects/myncctest
> ncc build src/index.ts --minify

ncc: Version 0.34.0
ncc: Compiling file index.js into ESM
ncc: Using [email protected] (local user-provided)
0kB  dist/package.json
0kB  dist/types.d.ts
0kB  dist/index.d.ts
1kB  dist/index.js
1kB  [1404ms] - ncc 0.34.0
pnpm run build  2.95s user 0.14s system 166% cpu 1.854 total

With this PR, it consistently takes 7-12 seconds. Command is identical to above: ncc build src/index.ts --minify. I don't get type checking or type definition files.

➜  myncctest git:(master) ✗ rm -rf dist && time pnpm run build

> [email protected] build /home/chris/projects/myncctest
> ncc build src/index.ts --minify

ncc: Version 0.34.0
ncc: Compiling file index.js into ESM
0kB  dist/package.json
1kB  dist/index.js
1kB  [598ms] - ncc 0.34.0
pnpm run build  38.72s user 0.27s system 388% cpu 10.030 total

Setting the --target es2020 has no effect on the performance. Omitting --minify does shave off around 1 second.

My test set up is WSL (Ubuntu) on Windows 11, Node 16.14.2.

On the plus side, the PR produces the exact same output as the latest ncc, so my package works as expected.

@kwonoj
Copy link
Author

kwonoj commented Jul 29, 2022

performance regression

The only thing I can think of is warmup time to compile wasm modules (@swc/wasm), but that probably won't take those huge time unless we spawn bunch of different process and each of them compiles wasm binary.

src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated
if(curPath.endsWith(".d.ts")) {
const outDir = tsconfig.compilerOptions.outDir ? pathResolve(tsconfig.compilerOptions.outDir) : pathResolve('dist');
if (curPath.endsWith(".d.ts")) {
const outDir = compilerOptions?.compilerOptions?.outDir ? pathResolve(compilerOptions.compilerOptions.outDir) : pathResolve('dist');
Copy link

Choose a reason for hiding this comment

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

What is compilerOptions?.compilerOptions for?

Copy link

Choose a reason for hiding this comment

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

The latest commit resolves the problem. The review can be marked as resolved now.

tsconfig = JSON5.parse(contents);
const baseUrl = tsconfig.compilerOptions.baseUrl;
resolveModules.push(pathResolve(dirname(configPath), baseUrl));
} catch (e) { }
Copy link

Choose a reason for hiding this comment

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

Should we show a warning message about missing/malformed tsconfig.json here, since compilerOptions would be an empty object and the default swc option would be used?

Copy link
Author

Choose a reason for hiding this comment

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

We didn't warn anything in any case https://github.com/vercel/ncc/pull/926/files#diff-bfe9874d239014961b1ae4e89875a6155667db834a410aaaa2ebe3cf89820556L136 , I'd consider this is failsafe same as before.

@kwonoj
Copy link
Author

kwonoj commented Aug 10, 2022

@styfle I've updated PR and resolved most, but can't understand test failures on the ci (https://github.com/vercel/ncc/runs/7774142054?check_suite_focus=true#step:9:303)

any possible suggestion to dig / fix this?

@@ -0,0 +1,110 @@
// Source code from https://github.com/swc-project/swc-loader/blob/master/src/index.js
// with the following changes:
// - swapped out `@swc/core` for `@swc/wasm`
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to use @swc/wasm instead of @swc/core? Seems like this was the cause of the slowdown that @cb1kenobi mentioned in #926 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, see comment (#926 (comment)) for the explanation.

Honestly, I do not know the exact reason. Previous work in here was using it (https://github.com/vercel/ncc/pull/777/files#diff-8c08ac17cfa691b1772e338822ad7b9cada2a78c0851239d4af55c561c943001R3), and per https://github.com/vercel/ncc/pull/777/files#diff-a992dd64a8c626fedc95d613f43602b91f6dc6339893afaf405dc05fa13c9ceaR96 I only could guess per-platform native binaries are not working well with how ncc works.

Choose a reason for hiding this comment

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

FWIW, I rebooted and pulled the latest and build times have dropped from 10 seconds down to 5.5 seconds on average. Mind you the latest ncc release builds in around 2 seconds, so there is definitely a performance regression.

it(`should execute "ncc ${(cliTest.args || []).join(" ")}"`, async () => {
const ps = fork(join(__dirname, file), cliTest.args || [], {
describe('cli', () => {
it.each(cliTests)('should execute ncc $args', async ({ args, env, expect: e, timeout }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Anything change here or is this just formatting?

Copy link
Author

@kwonoj kwonoj Aug 10, 2022

Choose a reason for hiding this comment

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

Formatting only to use jest's built in it.each, which provides better ergonomics / outputs.

Copy link
Member

Choose a reason for hiding this comment

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

I just wasn't sure if this could cause issues with the failing tests but seems unrelated

Copy link
Author

Choose a reason for hiding this comment

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

I believe this shouldn't affect test emission. Also actual failure is unit, cli.test seems passing fine.

Copy link
Member

Choose a reason for hiding this comment

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

This part seems concerning:

    - module.exports = eval("require")("../../../../../../../.././module");
    + module.exports = eval("require")("../../../../../../../../.././module");

https://github.com/vercel/ncc/runs/7774142054?check_suite_focus=true#step:9:366

That seems like its incorrect

Copy link
Author

Choose a reason for hiding this comment

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

Just for the update purpose - I tried a couple like cache clean and some others, but so far no luck. I'll try to reattempt when I have some time.

Copy link
Member

Choose a reason for hiding this comment

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

@kwonoj I can reproduce locally (macOS + Node.js 16.17.0) by checking out your branch and running

yarn install
yarn build
yarn test

image

Copy link
Author

Choose a reason for hiding this comment

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

I tried various, but really stuck with test result inconsistences between local to CI / other machines. It is possible my machine have some weird setup cauing problems, but without figuring it out I can't reliably say PR is ready to go.

Copy link
Member

Choose a reason for hiding this comment

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

Do you have docker locally? You could try cloning this branch in a docker container to see if that reproduces.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think I have a commercial license to install docker on the work machines. I'll think about a few, however I'm very curious why this only (yet) occurs on my machine. Wiping out whole repo & reclone even didn't make changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants