feat: loader support custom extension #156

whxaxes · 2018-03-20T09:59:38Z

Checklist

npm test passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

Description of change

loader 很多地方都是直接 hardcode 写死了加载 js，现在改成根据 require.extension 来决定加载哪些代码文件。

从而能够支持 ts-node 这类拓展 require.extension 来实现编译文件不落地的模块。

codecov · 2018-03-20T10:03:30Z

Codecov Report

Merging #156 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #156   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          17     17           
  Lines         887    893    +6     
=====================================
+ Hits          887    893    +6

Impacted Files	Coverage Δ
lib/egg.js	`100% <ø> (ø)`	⬆️
lib/loader/mixin/plugin.js	`100% <100%> (ø)`	⬆️
lib/loader/mixin/config.js	`100% <100%> (ø)`	⬆️
lib/loader/mixin/extend.js	`100% <100%> (ø)`	⬆️
lib/loader/file_loader.js	`100% <100%> (ø)`	⬆️
lib/utils/index.js	`100% <100%> (ø)`	⬆️
lib/loader/mixin/custom.js	`100% <100%> (ø)`	⬆️
lib/loader/mixin/router.js	`100% <100%> (ø)`	⬆️
lib/loader/egg_loader.js	`100% <100%> (ø)`	⬆️

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 9370f86...8f2c5b3. Read the comment docs.

popomore · 2018-03-20T12:23:48Z

lib/loader/file_loader.js

@@ -23,6 +23,7 @@ const defaults = {
  override: false,
  inject: undefined,
  filter: null,
+  extensions: [ '.js', '.ts' ],


这里支持 !.d.ts ?

popomore · 2018-03-20T12:26:31Z

lib/loader/file_loader.js

-    let files = this.options.match || [ '**/*.js' ];
-    files = Array.isArray(files) ? files : [ files ];
+    let files = this.options.match;
+    if (!files) {


这里不能用原来的逻辑？

如果用了 defaultMatch，就不需要再做 isArray 的判断了吧，因为就已经是 Array 了

atian25 · 2018-03-20T12:27:57Z

lib/loader/file_loader.js

-    files = Array.isArray(files) ? files : [ files ];
+    let files = this.options.match;
+    if (!files) {
+      files = this.defaultMatch;


如果这样的话，是不是干脆配置下默认的 match 就好了，不用 extensions?

大概想扩展吧，比如 jsx 啥的

那也是放到默认的 match 吧，现在这样子不就是根据 extensions 指定生成 match 么
那还不如直接让开发者写 match 覆盖即可

不过这里，要看怎么根据上层传递的值来开启 TS，否则我担心会 break，现在我们打包的时候好像 TS 和 JS 都打包进去了？

@whxaxes 把你 require.extensions 贴贴 @popomore 看

我想了又想，也觉得不该用 require.extensions，毕竟是已经弃用了。。。

可以的，改成 typescript

在 application 中提供一个 extensions 可供配置允许加载的后缀名会不会更好一些？方便拓展 jsx、mjs 等

要么就根据配置的后缀加载所有文件，然后根据是否扩展 require.extension 判断是否使用这个文件，比如 ts 通过 egg-cluster 开起 ts extension，这时就会加载 ts 了，而运行时如果只使用 js 就去除扩展 extension

你看他上面那个链接就是 require.extensions，这个方式还行，我只是担心这个 api 已经被标注为废弃

如果可以用 require.extensions，我就回滚到此前的 commit.(7c202b2)

popomore · 2018-03-20T12:29:48Z

没测试

whxaxes · 2018-03-20T13:05:16Z

@popomore 测试还没加好，所以还是 wip，先发上来给你们看看

popomore · 2018-03-20T15:47:04Z

可以在 application 加个 typescript 参数，开始这个后缀

whxaxes · 2018-03-21T10:35:27Z

单元测试已经补充，还是改回了通过 require.extensions 的方式。。

atian25

如果这样的话， typescript: true 都不用了。

popomore · 2018-03-21T15:41:46Z

不加参数的话怕不兼容，有些 ssr 可能做了处理

popomore

需要测试 ts 和 js 同时存在的场景

popomore · 2018-03-21T15:44:50Z

package.json

@@ -35,6 +35,7 @@
  "devDependencies": {
    "autod": "^3.0.1",
    "coffee": "^4.1.0",
+    "egg": "^2.5.0",


实现在载 ts 就好，不要依赖 egg

这个是写 fixtures 的时候引入了 egg 的 types...

我看一下怎么去掉先

如果去掉的话，就只能按照 js 的方式来写 ts 了...

测试的时候自己加一个 ts 的 extension 好了，断言转后的是否符合预期。

这里只要测 loader 就好了，另外可以在 egg 里加个用例

OK，我改一下

atian25 · 2018-03-21T15:49:36Z

不加参数的话怕不兼容，有些 ssr 可能做了处理

你指那些加了 babel register 的？

popomore · 2018-03-21T16:04:35Z

我不确定，暂时我就支持 ts 吧，也不需要做的太扩展。

atian25 · 2018-03-21T16:07:01Z

嗯，所以这里的修改意见是？

传递 typescript: true
不用 require.extensions
测试不用 egg

popomore · 2018-03-21T16:15:12Z

现在修改的地方只有两个

使用 glob 遍历文件是否包含这些后缀
判断后缀是否使用 require

这两个地方应该保持一致，所以遍历的文件的地方可以改成互斥的，如果 typescript: true 只加载 ts 文件。require 的时候需要确保定义了 extension，可以加个断言，�判断倒是还可以用 require.extensions

atian25 · 2018-03-21T16:23:57Z

typescript: true 只加载 ts

你指的是优先加 ts？

我觉得如果有同名的话，现在他的处理是优先加 js，这样才对

popomore · 2018-03-21T16:35:51Z

是只加载 ts，不加载 js

whxaxes · 2018-03-22T01:38:17Z

只加载 ts ？那引入的那些用 js 写的插件呢，如果使用 require.extensions 就是，只有在使用了 ts node 的情况下，才会去加载 ts，否则根本都不会去 load ts

whxaxes · 2018-03-23T02:56:12Z

已经改成使用 typescript 参数了，再看一下？

atian25 · 2018-03-23T05:33:51Z

lib/egg.js

@@ -28,6 +28,7 @@ class EggCore extends KoaApplication {
   * @param {Object} options - options
   * @param {String} [options.baseDir=process.cwd()] - the directory of application
   * @param {String} [options.type=application|agent] - whether it's running in app worker or agent worker
+   * @param {Boolean} [options.typescript] - whether support typescript


egg 的 d.ts 有没有这个的，回头要顺便加下

这个 typescript 会加载吧，不需要 loader 吧

这个 typescript 会加载吧，不需要 loader 吧

这个意思是？

这个文件不需要我们去 require

atian25 · 2018-03-23T05:35:49Z

lib/loader/file_loader.js

@@ -48,6 +50,7 @@ class FileLoader {
  constructor(options) {
    assert(options.directory, 'options.directory is required');
    assert(options.target, 'options.target is required');
+    assert(!options.typescript || (options.typescript && require.extensions['.ts']), 'require.extensions should contains .ts while options.typescript was true');


require.extensions / options.typescript 用 ` 引起来

atian25 · 2018-03-23T05:38:20Z

test/egg-ts.test.js

+  });
+
+  it('should support load ts,js files', async () => {
+    require.extensions['.ts'] = require.extensions['.js'];


为啥不放 afterEach

其实在这个文件里面, 可以放 before 和 after 里面, 一次性即可

可以，我改一下

atian25 · 2018-03-23T05:40:25Z

@popomore 再看看

popomore · 2018-03-23T03:40:54Z

lib/loader/file_loader.js

@@ -48,6 +50,7 @@ class FileLoader {
  constructor(options) {
    assert(options.directory, 'options.directory is required');
    assert(options.target, 'options.target is required');
+    assert(!options.typescript || (options.typescript && require.extensions['.ts']), 'require.extensions should contains .ts while options.typescript was true');


if (options.typescript) assert(require.extensions['.ts'])

这样？如果太长可以换行

OK，我改一下

popomore · 2018-03-23T06:23:49Z

其他 +1

whxaxes · 2018-03-23T06:53:26Z

已更新

whxaxes · 2018-03-23T07:29:01Z

@popomore 看一下刚提交的这段修改：46dc810

因为在 mixin 中很多是直接调用 loadFile 的，而 loadFile 中获取加载的文件路径是通过 utils.resolveModule 获得的，也就是当 typescript 为 false，但是如果在 require.extensions 中包含
.ts 的话，loadFile 中也是有可能传进来 .ts 的文件，所以加了个判断。

还有一个更改就是 extend.js 中也改成使用 egg_loader 的 loadFile，将 mixin 中直接 load 代码的逻辑尽量统一到 egg_loader 上的 loadFile 上来。

这样改后，就是当 options.typescript 为 false 的时候，不管是 file_loader，还是直接调用 egg_loader 的 loadFile，都不会去加载 .ts 文件

你看看这个处理合理不？

popomore · 2018-03-23T08:42:16Z

lib/loader/egg_loader.js

@@ -287,6 +287,10 @@ class EggLoader {
      return null;
    }

+    if (!this.options.typescript && filepath.endsWith('.ts')) {


这样看是不是在 resolveModule 处理比较好

resolveModule 是 utils 里的方法，要拿到 options 比较麻烦，得每一个调用都去传参，或者我把 resolveModule 移到 egg_loader 这边来？

会用到 resolveModule 的都是 mixin 里的方法，感觉可以移到 egg_loader 上

这样下面那个判断可以去掉了

OK，我改一下

popomore · 2018-03-23T08:45:19Z

其他同意

atian25 · 2018-03-23T09:56:05Z

ci 挂

whxaxes · 2018-03-23T10:02:56Z

ci 已经修好，已经将 resolveModule 移到 egg_loader 上，@atian25 @popomore 再看一下？

popomore

可以了

atian25 · 2018-03-25T10:49:09Z

这个发版本没？

popomore · 2018-03-25T11:25:41Z

egg-core@4.5.0

* feat: loader support custom extension * feat: remove require.extensions * refactor: refactor to require.extensions * test: add unittest for ts * chore: remove tsconfig.json * test: add d.ts * test: test loadCustomApp and loadCustomAgent * test: add more test for custom extend * fix: spelling mistake * feat: add typescript options * docs: update typescript opt to docs * chore: update comment * test: add more unittest for ts * refactor: code optimization * chore: update error msg * test: change beforeEach/afterEach to before/after * feat: add ts check in loadFile * feat: move resolveModule to egg_loader * fix: lint fix * refactor: code optimization

feat: loader support custom extension

7c202b2

whxaxes requested review from atian25 and popomore March 20, 2018 09:59

feat: remove require.extensions

030e489

popomore reviewed Mar 20, 2018

View reviewed changes

atian25 reviewed Mar 20, 2018

View reviewed changes

whxaxes added 3 commits March 21, 2018 16:19

refactor: refactor to require.extensions

49d967e

test: add unittest for ts

e710e8c

chore: remove tsconfig.json

1073043

test: add d.ts

b9795ca

whxaxes changed the title ~~[WIP]feat: loader support custom extension~~ feat: loader support custom extension Mar 21, 2018

whxaxes added 3 commits March 21, 2018 18:54

test: test loadCustomApp and loadCustomAgent

e718758

test: add more test for custom extend

c85b114

fix: spelling mistake

88117fb

atian25 approved these changes Mar 21, 2018

View reviewed changes

popomore reviewed Mar 21, 2018

View reviewed changes

whxaxes added 2 commits March 23, 2018 10:49

chore: update comment

12307ec

test: add more unittest for ts

8ce0beb

atian25 approved these changes Mar 23, 2018

View reviewed changes

popomore self-assigned this Mar 23, 2018

popomore approved these changes Mar 23, 2018

View reviewed changes

whxaxes added 2 commits March 23, 2018 14:30

refactor: code optimization

9b26274

chore: update error msg

c37fdb9

whxaxes added 2 commits March 23, 2018 14:56

test: change beforeEach/afterEach to before/after

5363b4a

feat: add ts check in loadFile

46dc810

popomore reviewed Mar 23, 2018

View reviewed changes

feat: move resolveModule to egg_loader

50d2d6e

whxaxes added 2 commits March 23, 2018 17:58

fix: lint fix

c51ae2d

refactor: code optimization

8f2c5b3

popomore approved these changes Mar 23, 2018

View reviewed changes

popomore merged commit 2c6fbbf into master Mar 23, 2018

popomore deleted the ts-node-support branch March 23, 2018 11:29

njugray mentioned this pull request Mar 26, 2018

fix: prevent lib/core/agent/index.js unexpectly loaded by plugin lo… alibaba/beidou#35

Merged

atian25 mentioned this pull request Mar 26, 2018

[RFC] TypeScript tool support eggjs/egg#2272

Closed

28 tasks

whxaxes mentioned this pull request Apr 12, 2018

Egg TS 改造总结 whxaxes/blog#12

Open

feat: loader support custom extension #156

feat: loader support custom extension #156

Conversation

whxaxes commented Mar 20, 2018 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

codecov bot commented Mar 20, 2018 • edited Loading

Codecov Report

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

whxaxes Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

popomore commented Mar 20, 2018

whxaxes commented Mar 20, 2018

popomore commented Mar 20, 2018

whxaxes commented Mar 21, 2018

atian25 left a comment

Choose a reason for hiding this comment

popomore commented Mar 21, 2018

popomore 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atian25 commented Mar 21, 2018

popomore commented Mar 21, 2018

atian25 commented Mar 21, 2018

popomore commented Mar 21, 2018

atian25 commented Mar 21, 2018

popomore commented Mar 21, 2018

whxaxes commented Mar 22, 2018

whxaxes commented Mar 23, 2018

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

atian25 commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popomore commented Mar 23, 2018

whxaxes commented Mar 23, 2018

whxaxes commented Mar 23, 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

popomore commented Mar 23, 2018

atian25 commented Mar 23, 2018

whxaxes commented Mar 23, 2018

popomore left a comment

Choose a reason for hiding this comment

atian25 commented Mar 25, 2018

popomore commented Mar 25, 2018

whxaxes commented Mar 20, 2018 •

edited

Loading

codecov bot commented Mar 20, 2018 •

edited

Loading

whxaxes Mar 21, 2018 •

edited

Loading

whxaxes commented Mar 23, 2018 •

edited

Loading