Ncc: Change source code to TypeScript

[styfle]

We like TypeScript. Let's write the source code for ncc in TypeScript.

Note: default exports are painful to deal with CJS/ESM interop so maybe use a named export instead.

[fkhadra]

Hey are you looking for help ?

[styfle]

@fkhadra Yes, if you are interested.

I was going to use this tsconfig.json:

{
  "compilerOptions": {
    "declaration": true,
    "esModuleInterop": true,
    "lib": ["esnext"],
    "module": "commonjs",
    "moduleResolution": "node",
    "noEmitOnError": true,
    "noFallthroughCasesInSwitch": true,
    "noImplicitReturns": true,
    "noUnusedLocals": true,
    "noUnusedParameters": true,
    "outDir": "./dist",
    "types": ["node"],
    "strict": true,
    "target": "esnext",
    "typeRoots": [
      "./types",
      "./node_modules/@types"
    ]
  }
}

Do you want to make a PR?

[axetroy]

There is no doubt that Typescript is undoubtedly the best programming method at this moment.

you can consider refactoring with Typescript before project become bigger and bigger.

[fkhadra]

Hey @styfle sure for the PR, it would be nice. Is there a commit convention that I have to follow ? Thank you

[styfle]

@fkhadra No, we squash on merge

[fkhadra]

Hey @styfle, I started to work on it. https://github.com/fkhadra/ncc/tree/move-to-ts

I took your tsconfig and changed the outDir to ’./tsbuild’ just the time to migrate all the code base.

When I migrate a file to typescript I require the compiled version as shown below then I run all the test suite to ensure that nothing is broken.

const { shebangRegExp } = require('../tsbuild/utils/shebang');

I saw that the default export of get-package-base file was not used so I removed it.

I have migrated the files inside the src/utils directory to TS at that time.

What do you think about that workflow ? Do you have a better idea ? Thank you

[styfle]

@fkhadra That sound about right. Can you submit the PR so I can review?

[styfle]

FYI, we don't need to change the source code of tests right now. Let's prove that tests still work without changing them.

[fkhadra]

@styfle I underestimated the work needed to migrate to typescript 😭, I think that some task needs to be done up front.

There are 2 tests that fail when I add the latest version of typescript 3.4.1 or @types/node. The test that failing are the one related to typescript. I'm trying to find why.

The issue can be reproduced as follow:

• clone the repository(wanted to get a fresh copy)
• yarn (install packages)
• yarn test
  
• yarn add typescript@latest
• yarn test
  

Ncc uses arg v2, we can upgrade to the latest version(v4) without touching a single line of code. The nice thing is that the v4 ship with a ts definition file. I can make a separate PR for that one if you want to.

PS: Should I open an issue for the typescript dependency ?

[styfle]

Yes please upgrade arg, thanks! 👍

[styfle]

@fkhadra Any update on this?

We had to add types to the node builder recently because ncc doesn't expose any 😢

https://github.com/zeit/now-builders/blob/375874e9894facb87dfc7e1dd254a6ad5a919dd9/packages/now-node/src/watcher.ts

[fkhadra]

Hey @styfle, I still have the issue https://github.com/zeit/ncc/issues/343#issuecomment-480042541 regarding the tests.
I don't know what I'm missing about that one. Do you have any idea ?

[styfle]

@fkhadra I thought the problem was with arg but I guess that was not related to the test failures.

Where is the repo? The link you posted above is dead: https://github.com/fkhadra/ncc/tree/move-to-ts

Maybe you could submit a PR?

[fkhadra]

@styfle you can reproduce the issue with the current master branch. You don't even need to change a single line of code :(.

I deleted my repo since I wanted to fix this issue first.

[fkhadra]

Hey @styfle were you able to reproduce the issue ?

[styfle]

@fkhadra Sorry, too busy to look into this one since its lower priority right now.

[fabiospampinato]

Note: default exports are painful to deal with CJS/ESM interop so maybe use a named export instead.

FYI, if I'm understanding this correctly, I have a bit of a package that allows default exports to be imported normally, with require ( 'pkgname' ), under CJS.