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

feat: loader support custom extension #156

Merged
merged 20 commits into from Mar 23, 2018

Conversation

Projects
None yet
3 participants
@whxaxes
Copy link
Member

whxaxes commented Mar 20, 2018

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 来实现编译文件不落地的模块。

@whxaxes whxaxes requested review from atian25 and popomore Mar 20, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 20, 2018

Codecov Report

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

Impacted file tree graph

@@          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.

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

This comment has been minimized.

@popomore

popomore Mar 20, 2018

Member

这里支持 !.d.ts ?

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

This comment has been minimized.

@popomore

popomore Mar 20, 2018

Member

这里不能用原来的逻辑?

This comment has been minimized.

@whxaxes

whxaxes Mar 20, 2018

Member

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

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

This comment has been minimized.

@atian25

atian25 Mar 20, 2018

Member

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

This comment has been minimized.

@popomore

popomore Mar 20, 2018

Member

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

This comment has been minimized.

@atian25

atian25 Mar 20, 2018

Member

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

This comment has been minimized.

@atian25

atian25 Mar 20, 2018

Member

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

@whxaxes 把你 require.extensions 贴贴 @popomore

This comment has been minimized.

@whxaxes

whxaxes Mar 20, 2018

Member

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

This comment has been minimized.

@popomore

popomore Mar 20, 2018

Member

可以的,改成 typescript

This comment has been minimized.

@whxaxes

whxaxes Mar 20, 2018

Member

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

This comment has been minimized.

@popomore

popomore Mar 20, 2018

Member

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

This comment has been minimized.

@atian25

atian25 Mar 20, 2018

Member

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

This comment has been minimized.

@whxaxes

whxaxes Mar 21, 2018

Member

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

@popomore

This comment has been minimized.

Copy link
Member

popomore commented Mar 20, 2018

没测试

@whxaxes

This comment has been minimized.

Copy link
Member

whxaxes commented Mar 20, 2018

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

@popomore

This comment has been minimized.

Copy link
Member

popomore commented Mar 20, 2018

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

@whxaxes

This comment has been minimized.

Copy link
Member

whxaxes commented Mar 21, 2018

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

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

whxaxes added some commits Mar 21, 2018

@atian25
Copy link
Member

atian25 left a comment

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

@popomore

This comment has been minimized.

Copy link
Member

popomore commented Mar 21, 2018

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

@popomore
Copy link
Member

popomore left a comment

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

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

This comment has been minimized.

@popomore

popomore Mar 21, 2018

Member

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

This comment has been minimized.

@whxaxes

whxaxes Mar 22, 2018

Member

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

This comment has been minimized.

@whxaxes

whxaxes Mar 22, 2018

Member

我看一下怎么去掉先

This comment has been minimized.

@whxaxes

whxaxes Mar 22, 2018

Member

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

This comment has been minimized.

@popomore

popomore Mar 22, 2018

Member

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

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

This comment has been minimized.

@whxaxes

whxaxes Mar 22, 2018

Member

OK,我改一下

@atian25

This comment has been minimized.

Copy link
Member

atian25 commented Mar 21, 2018

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

你指那些加了 babel register 的?

@popomore

This comment has been minimized.

Copy link
Member

popomore commented Mar 21, 2018

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

@atian25

This comment has been minimized.

Copy link
Member

atian25 commented Mar 21, 2018

嗯,所以这里的修改意见是?

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

This comment has been minimized.

Copy link
Member

popomore commented Mar 21, 2018

现在修改的地方只有两个

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

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

@atian25

This comment has been minimized.

Copy link
Member

atian25 commented Mar 21, 2018

typescript: true 只加载 ts

你指的是优先加 ts?

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

@popomore

This comment has been minimized.

Copy link
Member

popomore commented Mar 21, 2018

是只加载 ts,不加载 js

@whxaxes

This comment has been minimized.

Copy link
Member

whxaxes commented Mar 22, 2018

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

whxaxes added some commits Mar 23, 2018

@whxaxes

This comment has been minimized.

Copy link
Member

whxaxes commented Mar 23, 2018

已经改成使用 typescript 参数了,再看一下?

@@ -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

This comment has been minimized.

@atian25

atian25 Mar 23, 2018

Member

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

This comment has been minimized.

@popomore

popomore Mar 23, 2018

Member

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

This comment has been minimized.

@whxaxes

whxaxes Mar 23, 2018

Member

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

这个意思是?

This comment has been minimized.

@popomore

popomore Mar 23, 2018

Member

这个文件不需要我们去 require

@@ -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');

This comment has been minimized.

@atian25

atian25 Mar 23, 2018

Member

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

});

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

This comment has been minimized.

@atian25

atian25 Mar 23, 2018

Member

为啥不放 afterEach

This comment has been minimized.

@atian25

atian25 Mar 23, 2018

Member

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

This comment has been minimized.

@whxaxes

whxaxes Mar 23, 2018

Member

可以,我改一下

@atian25

This comment has been minimized.

Copy link
Member

atian25 commented Mar 23, 2018

@popomore 再看看

@popomore popomore self-assigned this Mar 23, 2018

@@ -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');

This comment has been minimized.

@popomore

popomore Mar 23, 2018

Member

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

这样?如果太长可以换行

This comment has been minimized.

@whxaxes

whxaxes Mar 23, 2018

Member

OK,我改一下

@popomore

This comment has been minimized.

Copy link
Member

popomore commented Mar 23, 2018

其他 +1

whxaxes added some commits Mar 23, 2018

@whxaxes

This comment has been minimized.

Copy link
Member

whxaxes commented Mar 23, 2018

已更新

@whxaxes

This comment has been minimized.

Copy link
Member

whxaxes commented Mar 23, 2018

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

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

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

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

你看看这个处理合理不?

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

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

This comment has been minimized.

@popomore

popomore Mar 23, 2018

Member

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

This comment has been minimized.

@whxaxes

whxaxes Mar 23, 2018

Member

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

This comment has been minimized.

@whxaxes

whxaxes Mar 23, 2018

Member

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

This comment has been minimized.

@popomore

popomore Mar 23, 2018

Member

可以的

This comment has been minimized.

@popomore

popomore Mar 23, 2018

Member

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

This comment has been minimized.

@whxaxes

whxaxes Mar 23, 2018

Member

OK,我改一下

@popomore

This comment has been minimized.

Copy link
Member

popomore commented Mar 23, 2018

其他同意

@atian25

This comment has been minimized.

Copy link
Member

atian25 commented Mar 23, 2018

ci 挂

@whxaxes

This comment has been minimized.

Copy link
Member

whxaxes commented Mar 23, 2018

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

@popomore
Copy link
Member

popomore left a comment

可以了

@popomore popomore merged commit 2c6fbbf into master Mar 23, 2018

8 checks passed

WIP ready for review
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to 9370f86
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
security/snyk No dependency changes
Details

@popomore popomore deleted the ts-node-support branch Mar 23, 2018

@atian25

This comment has been minimized.

Copy link
Member

atian25 commented Mar 25, 2018

这个发版本没?

@popomore

This comment has been minimized.

Copy link
Member

popomore commented Mar 25, 2018

  • egg-core@4.5.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment