-
Notifications
You must be signed in to change notification settings - Fork 288
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR! I've testing on real repos here: This needs further investigation before we land since its not clear why the |
Agreed, somehow only on swc it try to replace it into bundled require logic. I may need further digging to find out reason. |
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. |
@kwonoj Hmm, I think replacing tsloader with swc is a fair tradeoff to losing Please update this PR to fix the conflicts and then we can merge, thanks! |
2615a20
to
dd6053b
Compare
ts-loader
with swc
ts-loader
with swcts-loader
with swc
}, | ||
{ | ||
args: ["run", "-t", "test/fixtures/with-type-errors/ts-error.ts"], | ||
expect: { code: 0 } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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, I don't know if it is the case, so I will raise it here. |
This is true, but afaik the issue in here I observed was not this one. |
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 The latest
With this PR, it consistently takes 7-12 seconds. Command is identical to above:
Setting the 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 |
The only thing I can think of is warmup time to compile wasm modules ( |
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'); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) { } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@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` |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 }) => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Breaking Changes
tsc
instead.d.ts
files, runtsc
insteadDescription
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.
swc-loader
for TypeScript compilation #777