TypeScript #90

rauchg · 2018-12-03T05:05:25Z

This PR:

adds a conditionally loaded ts-loader.js file
- bundles typescript
- this means non-typescript users will only pay a download penalty, not not bootup time penalty
adds an integration test
- adds tsconfig.json under test/integration/ which is a requirement
prettifies the codebase. To simplify reviewing, I'm annotating my changes with GitHub comments

Closes #31

rauchg · 2018-12-03T05:10:12Z

scripts/build.js

+
+  const { code: tsLoader, assets: tsLoaderAssets } = await ncc(
+    __dirname + "/../src/ts-loader"
+  );


rauchg · 2018-12-03T05:10:16Z

scripts/build.js

+  if (
+    Object.keys(cliAssets).length ||
+    Object.keys(indexAssets).length ||
+    Object.keys(tsLoaderAssets).length ||


rauchg · 2018-12-03T05:10:20Z

scripts/build.js

  }

  writeFileSync(__dirname + "/../dist/ncc/cli.js", cli);
  writeFileSync(__dirname + "/../dist/ncc/index.js", index);
+  writeFileSync(__dirname + "/../dist/ncc/ts-loader.js", tsLoader);


rauchg · 2018-12-03T05:10:27Z

src/index.js


-const SUPPORTED_EXTENSIONS = [".js", ".mjs", ".json"];
+const SUPPORTED_EXTENSIONS = [".ts", ".tsx", ".js", ".mjs", ".json"];


Do we want to support ".tsx" or just ".ts"? Just seems like then we should be supporting ".jsx" as well?

@guybedford .tsx is officially supported by TypeScript, but .jsx is not (by either TS or Node)

I understand that, it just seems a little inconsistent to me that jsx can be supported through TS only... I don't think Deno supports tsx for example.

rauchg · 2018-12-03T05:10:36Z

src/index.js

-          test: /\.(js|mjs)/,
+          test: /\.tsx?$/,
+          use: [{ loader: __dirname + "/ts-loader.js" }]
+        },


rauchg · 2018-12-03T05:10:43Z

src/ts-loader.js

+// optional bundle for the ts-loader, that
+// doesn't get loaded unless the user is
+// compiling typescript
+module.exports = require("ts-loader");


rauchg · 2018-12-03T05:10:53Z

test/index.test.js

+      module.exports && module.exports.default
+        ? module.exports.default
+        : module.exports;
+    if ("function" !== typeof exp) {


rauchg · 2018-12-03T05:10:57Z

test/index.test.js

  }
 }

 for (const integrationTest of fs.readdirSync(__dirname + "/integration")) {
  // ignore e.g.: `.json` files
-  if (!integrationTest.endsWith(".js")) continue;
+  if (!/\.(mjs|tsx?|js)$/.test(integrationTest)) continue;


rauchg · 2018-12-03T05:11:01Z

test/index.test.js

      throw new Error(
        `Integration test "${integrationTest}" evaluation failed. It does not export a function`
      );
    }
-    await module.exports();
+    await exp();


codecov-io · 2018-12-03T05:12:44Z

Codecov Report

Merging #90 into master will decrease coverage by 0.28%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
- Coverage   76.44%   76.15%   -0.29%     
==========================================
  Files           7        8       +1     
  Lines         433      432       -1     
==========================================
- Hits          331      329       -2     
- Misses        102      103       +1

Impacted Files	Coverage Δ
src/loaders/ts-loader.js	`0% <0%> (ø)`
src/index.js	`93.75% <100%> (-0.19%)`	⬇️

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 e8f0742...835171e. Read the comment docs.

guybedford

👍 👍

guybedford · 2018-12-03T13:00:28Z

src/index.js


-const SUPPORTED_EXTENSIONS = [".js", ".mjs", ".json"];
+const SUPPORTED_EXTENSIONS = [".ts", ".tsx", ".js", ".mjs", ".json"];


Do we want to support ".tsx" or just ".ts"? Just seems like then we should be supporting ".jsx" as well?

guybedford · 2018-12-03T13:03:53Z

readme.md

+    "target": "es2015"
+  }
+}
+```


I wonder if we could make the tsconfig.json optional with the above as the default?

Good observation. It's not recommended due to editor integration. In other words, e.g.: VSCode would need a mechanism to understand what rules ncc would be using.

It's a small extra step, but keeps all tooling in sync. Plus, depending on what Node.js version the developer is targeting, the target can be adjusted, which is quite nice.

Makes sense 👍

guybedford · 2018-12-05T15:23:31Z

I've gone ahead and rebased this PR - please test it again before merging though.

cztomsik · 2018-12-06T17:19:36Z

I can't get it working, I've cloned the repo (because it's not published yet), ran npm i && npm run build && npm link and yet I still get error about missing ts-loader.

I've also tried installing it in both ncc dir (and running npm run build again to regenerate ncc) and also in my test dir

Here's my file (test.tsx):

console.log('Hello')

And tsconfig.json:

{
  "compilerOptions": {
    "lib": ["es2017"],
    "target": "es2017",
    "module": "commonjs",
    "jsx": "react",
    "experimentalDecorators": true
  }
}

Here's what I get:

  ncc-tsx-test$ ncc build test.tsx 
Error: Hash: b30f5d0d1693b0400894
Version: webpack 4.27.1
Time: 141ms
Built at: 12/06/2018 6:16:46 PM
       Asset      Size  Chunks  Chunk Names
    index.js  3.94 KiB       0  main
index.js.map  3.45 KiB       0  main
Entrypoint main = index.js index.js.map
[0] ./test.tsx 322 bytes {0} [built] [failed] [1 error]

ERROR in ./test.tsx
Module build failed (from ../ncc/dist/ncc/loaders/ts-loader.js):
Error: Cannot find module '/Users/cztomsik/Desktop/playground/ncc/dist/ncc/loaders/ts-loader.js'
    at /Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:75024:9
    at process._tickCallback (internal/process/next_tick.js:68:7)
    at compiler.run (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:65133:23)
    at finalCallback (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:66089:39)
    at hooks.done.callAsync.err (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:66105:13)
    at AsyncSeriesHook.eval [as callAsync] (eval at create (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:15151:10), <anonymous>:6:1)
    at AsyncSeriesHook.lazyCompileHook (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:14592:20)
    at onCompiled (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:66103:21)
    at hooks.afterCompile.callAsync.err (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:66431:14)
    at AsyncSeriesHook.eval [as callAsync] (eval at create (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:15151:10), <anonymous>:6:1)
    at AsyncSeriesHook.lazyCompileHook (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:14592:20)
    at compilation.seal.err (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:66428:30)

guybedford · 2018-12-06T17:22:26Z

Perhaps see #118.

…

On Thu, 06 Dec 2018 at 19:19, Kamil Tomšík ***@***.***> wrote: I can't get it working, I've cloned the repo (because it's not published yet), ran npm i && npm link and yet I still get error about missing ts-loader. I've also tried installing it in both ncc dir (and running npm run build again to regenerate ncc) and also in my test dir Here's my file (test.tsx): console.log('Hello') And tsconfig.json: { "compilerOptions": { "lib": ["es2017"], "target": "es2017", "module": "commonjs", "jsx": "react", "experimentalDecorators": true } } Here's what I get: ncc-tsx-test$ ncc build test.tsx Error: Hash: b30f5d0d1693b0400894 Version: webpack 4.27.1 Time: 141ms Built at: 12/06/2018 6:16:46 PM Asset Size Chunks Chunk Names index.js 3.94 KiB 0 main index.js.map 3.45 KiB 0 main Entrypoint main = index.js index.js.map [0] ./test.tsx 322 bytes {0} [built] [failed] [1 error] ERROR in ./test.tsx Module build failed (from ../ncc/dist/ncc/loaders/ts-loader.js): Error: Cannot find module '/Users/cztomsik/Desktop/playground/ncc/dist/ncc/loaders/ts-loader.js' at /Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:75024:9 at process._tickCallback (internal/process/next_tick.js:68:7) at compiler.run (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:65133:23) at finalCallback (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:66089:39) at hooks.done.callAsync.err (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:66105:13) at AsyncSeriesHook.eval [as callAsync] (eval at create (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:15151:10), <anonymous>:6:1) at AsyncSeriesHook.lazyCompileHook (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:14592:20) at onCompiled (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:66103:21) at hooks.afterCompile.callAsync.err (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:66431:14) at AsyncSeriesHook.eval [as callAsync] (eval at create (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:15151:10), <anonymous>:6:1) at AsyncSeriesHook.lazyCompileHook (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:14592:20) at compilation.seal.err (/Users/cztomsik/Desktop/playground/ncc/dist/ncc/index.js:66428:30) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#90 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkiyjI3EsiNf0tDJyuRFoBdob5xb4kGks5u2VGogaJpZM4Y94na> .

cztomsik · 2018-12-07T20:32:40Z

thx, it helped

esamattis · 2018-12-10T13:55:19Z

bundles typescript

Is it possible use custom version of Typescript?

rauchg commented Dec 3, 2018

View reviewed changes

scripts/build.js

const { code: tsLoader, assets: tsLoaderAssets } = await ncc(

__dirname + "/../src/ts-loader"

);

Copy link

Member Author

rauchg Dec 3, 2018

Choose a reason for hiding this comment

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

Added this

rauchg commented Dec 3, 2018

View reviewed changes

src/index.js

test: /\.(js|mjs)/,

test: /\.tsx?$/,

use: [{ loader: __dirname + "/ts-loader.js" }]

},

Copy link

Member Author

rauchg Dec 3, 2018

Choose a reason for hiding this comment

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

Added this

rauchg commented Dec 3, 2018

View reviewed changes

guybedford approved these changes Dec 3, 2018

View reviewed changes

guybedford force-pushed the typescript branch from 2ca4a69 to 835171e Compare December 5, 2018 15:23

rauchg and others added 6 commits December 5, 2018 20:51

prettify and add ts-loader output

16361b8

add ts-loader.js

96ae4a9

prettify and add ts support

fcba7c8

prettify and add support for export default () {}

a423011

add readme

04e5187

add yarn lock

0b11972

guybedford force-pushed the typescript branch from 835171e to 0b11972 Compare December 5, 2018 18:52

rauchg merged commit 8032a35 into master Dec 5, 2018

rauchg deleted the typescript branch December 5, 2018 23:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TypeScript #90

TypeScript #90

rauchg commented Dec 3, 2018 •

edited

Loading

rauchg Dec 3, 2018

rauchg Dec 3, 2018

rauchg Dec 3, 2018

rauchg Dec 3, 2018

guybedford Dec 3, 2018

rauchg Dec 3, 2018

guybedford Dec 3, 2018

rauchg Dec 3, 2018

rauchg Dec 3, 2018

rauchg Dec 3, 2018

rauchg Dec 3, 2018

rauchg Dec 3, 2018

codecov-io commented Dec 3, 2018 •

edited

Loading

guybedford left a comment

guybedford Dec 3, 2018

guybedford Dec 3, 2018

rauchg Dec 3, 2018

guybedford Dec 3, 2018

guybedford commented Dec 5, 2018

cztomsik commented Dec 6, 2018 •

edited

Loading

guybedford commented Dec 6, 2018 via email

cztomsik commented Dec 7, 2018

esamattis commented Dec 10, 2018


		const SUPPORTED_EXTENSIONS = [".js", ".mjs", ".json"];
		const SUPPORTED_EXTENSIONS = [".ts", ".tsx", ".js", ".mjs", ".json"];

TypeScript #90

TypeScript #90

Conversation

rauchg commented Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Dec 3, 2018 • edited Loading

Codecov Report

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford commented Dec 5, 2018

cztomsik commented Dec 6, 2018 • edited Loading

guybedford commented Dec 6, 2018 via email

cztomsik commented Dec 7, 2018

esamattis commented Dec 10, 2018

rauchg commented Dec 3, 2018 •

edited

Loading

codecov-io commented Dec 3, 2018 •

edited

Loading

cztomsik commented Dec 6, 2018 •

edited

Loading