feat: refactor #2

ngot · 2017-03-13T08:51:15Z

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

codecov · 2017-03-13T08:51:47Z

Codecov Report

Merging #2 into master will increase coverage by 8%.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master     #2   +/-   ##
=====================================
+ Coverage      92%   100%   +8%     
=====================================
  Files           5      7    +2     
  Lines         100     71   -29     
=====================================
- Hits           92     71   -21     
+ Misses          8      0    -8

Impacted Files	Coverage Δ
lib/view.js	`100% <100%> (+5.26%)`	✅
lib/xtpl.js	`100% <100%> (ø)`
app/extend/application.js	`100% <100%> (ø)`
config/config.default.js	`100% <100%> (ø)`	✅
lib/engine.js	`100% <100%> (+9.58%)`	✅

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 e5dfd62...23bcaa8. Read the comment docs.

ngot · 2017-03-13T08:56:12Z

重构了一版

@popomore @atian25 看下
loadpath 本来的版本就没有效果的，根本走不到那个逻辑。

popomore · 2017-03-13T09:13:30Z

test/fixtures/apps/xtpl-demo/app/controller/string.js

@@ -3,5 +3,6 @@
 module.exports = function* () {
  this.body = yield this.renderString('foo {{ name }}', {
    name: 'ngot',
+    viewEngine: 'xtpl',


第三个参数

popomore · 2017-03-13T09:17:25Z

lib/xtpl.js

+
+const Xtemplate = require('xtemplate');
+const debug = require('debug')('egg-view-xtpl:xtpl');
+const fs = require('fs-then');


atian25 · 2017-03-13T09:12:01Z

app/extend/application.js

+'use strict';
+
+const XTPL = Symbol('app#XTPL');
+const ENGINE = require('../../lib/engine');


这个不是静态变量，不用全大写吧？

const engine = require('../../lib/engine');

我改成

const XTPLSYMBOL = Symbol('app#xtpl'); const engine = require('../../lib/engine');

吧

atian25 · 2017-03-13T09:13:03Z

app/extend/application.js

@@ -0,0 +1,18 @@
+'use strict';
+
+const XTPL = Symbol('app#XTPL');


这个用 const ENGINE = Symbol('app#xtpl') 感觉合适点

atian25 · 2017-03-13T09:13:33Z

app/extend/application.js

+
+  /**
+   * xtpl environment
+   * @member {XTPLEnvironment} Application#xtpl


XTPLEnvironment 这个是 xtpl 那边的 class name 拼写么?

这个不是，xtpl没有这个概念。我改成自定义的 xtpl 吧

atian25 · 2017-03-13T09:16:01Z

lib/xtpl.js

+const LRU = require('lru-cache');
+const copy = require('copy-to');
+
+const INSTANCECACHESYMBOL = Symbol('XTPL#INSTANCECACHE');


这几个全大写看起来好累.. SYMBOL 后缀没必要加了吧

const INSTANCE_CACHE = Symbol('XtplEngine#instanceCache');

下面几个类似

atian25 · 2017-03-13T09:17:10Z

lib/xtpl.js

+    return cached;
+  }
+
+  readFile(pathname, config) {


perfer pathName or fileName

atian25 · 2017-03-13T09:19:27Z

lib/xtpl.js

+    }
+
+    return new Promise((resolve, reject) => {
+      const instance = this.getInstance({


xtpl 实际业务中会有很多 instance?

一个request 一个 instance

atian25 · 2017-03-13T09:19:55Z

lib/xtpl.js

+    copy(self.defaultOptions).to(options);
+    pathname = path.normalize(pathname);
+
+    function load(tpl, callback) {


这函数能否变为私有成员函数? 不用每次都 new ?

atian25 · 2017-03-13T09:20:29Z

package.json

@@ -19,6 +19,7 @@
  "dependencies": {
    "copy-to": "^2.0.1",
    "debug": "^2.6.1",
+    "fs-then": "^0.2.0",


用 mz ? https://www.npmjs.com/package/mz

atian25 · 2017-03-13T09:20:54Z

test/fixtures/apps/xtpl-demo/app/view/gbk.xtpl

@@ -0,0 +1 @@
+< ��� {{ name  }}


这个是 gbk

atian25 · 2017-03-13T09:21:36Z

test/fixtures/apps/xtpl-cache/app/controller/fileCache.js

+  yield this.render('home.xtpl', {
+    name: 'ngot',
+  });
+  this.app.xtpl.fnCache.reset();


要支持个 cleanCache() 么?

暂时，貌似还没有什么用。以后支持吧

popomore · 2017-03-13T09:32:52Z

lib/xtpl.js

+    const cache = config.cache;
+    const pathname = config.name;
+    let cached;
+    if (cache && (cached = self.instanceCache.peek(pathname))) {


为啥用 self？

跟着其他几处，顺手写的，我改掉吧

popomore · 2017-03-13T09:33:18Z

lib/xtpl.js

+    return this[INSTANCECACHESYMBOL];
+  }
+
+  get fileCache() {


这几个为啥不再构造函数里面

popomore · 2017-03-13T09:35:37Z

lib/xtpl.js

+  }
+
+  renderString(str, data, options) {
+    copy(this.defaultOptions).to(options);


非递归的用 Object.assign 就好了？

这里默认值，复制利用 copy比较方便一些

和 Object.assign 一个意思吧？

等价于 Object.assign({}, defaultOptions, options);

popomore · 2017-03-13T09:36:17Z

lib/xtpl.js

+    copy(this.defaultOptions).to(options);
+    return new Promise((resolve, reject) => {
+      const fn = new Xtemplate(str);
+      fn.render(data, { commands: options.commands }, renderTpl);


这个会有异常么

会有的，比如语法错误，传入的扩展 commands 抛异常

那要 try/catch

按上面说的，改成 generator 吧

popomore · 2017-03-13T09:37:12Z

lib/xtpl.js

+
+      self.readFile(pathname, rootConfig)
+        .then(tpl => {
+          return new Promise((resolve, reject) => {


嵌套这么深还是用 generator 吧

popomore · 2017-03-13T09:40:22Z

lib/xtpl.js

+      const cache = rootConfig.cache;
+      debug('rootConfig:', rootConfig);
+
+      if (cache && self.fnCache.has(pathname)) {


readFile 里有一层缓存了

popomore · 2017-03-13T14:09:24Z

lib/xtpl.js

-const INSTANCECACHESYMBOL = Symbol('XTPL#INSTANCECACHE');
-const FNCACHESYMBOL = Symbol('XTPL#FNCACHE');
-const FILECACHESYMBOL = Symbol('XTPL#FILECACHE');
+const INSTANCECACHESYMBOL = Symbol.for('XTPL#INSTANCECACHE');


为啥用 Symbol.for

考虑给这个测试用的

egg-view-xtpl/test/fixtures/apps/xtpl-cache/app/controller/fileCache.js

Line 10 in b59e7d8

this.app.xtpl[FNCACHESYMBOL].reset();

fncache和filecache确实有些重复了。我把filecache去掉吧，只缓存编译后的结果。这个测试就不要了。

popomore · 2017-03-13T14:10:10Z

lib/xtpl.js


 class XtplEngine {
+  constructor() {
+    this[INSTANCECACHESYMBOL] = new LRU();


有个疑问，模板为什么要用 lru

如果按照现在这种使用方式，用 map也就够了。不过，lru的话，可以比较方便后续扩展设置缓存超时时间，实际使用上也没什么副作用。比较方便。

只是疑问为什么需要缓存超时，模板除非是文件变更，不太会变化的。

目前设置默认配置是不超时的，也没开放设置的

ngot · 2017-03-14T04:28:45Z

改了，大家再看看 @atian25 @popomore

atian25 · 2017-03-14T04:57:33Z

app/extend/application.js

@@ -0,0 +1,18 @@
+'use strict';
+
+const XTPLSYMBOL = Symbol('app#xtpl');


XTPL 就好了吧，不用加 SYMBOL

atian25 · 2017-03-14T04:59:07Z

lib/view.js

-      filename = path.join(this.config.loadpath, filename + '.xtpl');
-    }
-    return engine.render(filename, locals, config);
+    debug('filename:', filename);


debug('render filename:'

atian25 · 2017-03-14T05:33:33Z

lib/view.js

  }

  renderString(str, locals, viewOptions) {
-    assert(str, 'view string must be supplied!');
-    assert(locals, 'locals must be supplied!');
    const config = Object.assign({}, this.config, viewOptions, { cache: null });


ejs 那边加 cache: null 是因为 renderString 无法缓存，不这么做会报错， xtpl 这边也需要？

这个是没用的

atian25 · 2017-03-14T05:38:16Z

package.json

@@ -17,15 +17,15 @@
    "egg-plugin"


加一个 egg-view

atian25

其他 +1。 @popomore 再看看。

PS: 话说， xtpl 怎么设计得每一个 request 一个 instance，浪费啊。

atian25 · 2017-03-14T05:46:34Z

lib/view.js

@@ -12,12 +12,12 @@ class XTPLView {

  render(filename, locals, viewOptions) {
    const config = Object.assign({}, this.config, viewOptions, { filename });
-    debug('filename:', filename);
+    debug('render filename::', filename);


多了一个 :

atian25 · 2017-03-14T05:48:33Z

windows 用例挂了

ngot · 2017-03-14T06:31:39Z

@atian25 xtemplate自己在windows下，挂了...我提个issue吧，不影响插件自己

atian25 · 2017-03-14T06:38:10Z

+1

ngot · 2017-03-14T07:01:29Z

xtemplate/xtemplate#94

atian25 · 2017-03-14T07:21:38Z

@popomore 看看有没有问题，没的话可以先发个 minor 了

popomore · 2017-03-14T05:43:27Z

lib/xtpl.js

+
+const defaultOptions = {
+  catchError: process.env.NODE_ENV !== 'production',
+  encoding: 'utf-8',


这个不能通过 config.xtpl 配置？

我写的多余了。我删除了。

popomore · 2017-03-14T05:44:42Z

lib/view.js

-    }
-    return engine.render(filename, locals, config);
+    debug('filename:', filename);
+    return this.app.xtpl.render(filename, locals, config);


这个需要用 yield 了吧

popomore · 2017-03-14T08:05:35Z

lib/view.js

-    }
-    return engine.render(filename, locals, config);
+    debug('render filename:', filename);
+    return this.app.xtpl.render(filename, locals, config);


这个要用 yield 吧

这样可以的。yield的话，这个方法得改成 generator。

这个调整为generator了

popomore · 2017-03-14T08:11:15Z

其他 +1

ngot · 2017-03-14T08:42:14Z

我合并，发个minor

ngot · 2017-03-14T08:50:49Z

egg-view-xtpl@1.1.0

ngot added 2 commits March 13, 2017 12:23

refactor:simplify implement

98ef200

test: more test

f31d241

test: fix title

a173e27

popomore reviewed Mar 13, 2017

View reviewed changes

atian25 requested changes Mar 13, 2017

View reviewed changes

popomore reviewed Mar 13, 2017

View reviewed changes

chore:some fix

b59e7d8

popomore reviewed Mar 13, 2017

View reviewed changes

ngot added 2 commits March 14, 2017 12:11

refactor: simplfy implement

f950687

refactor: simplfy implement

06e20d1

refactor:performance

4807886

atian25 approved these changes Mar 14, 2017

View reviewed changes

chore:some adjustment

b6a5499

atian25 reviewed Mar 14, 2017

View reviewed changes

chore:some adjustment

6602819

popomore reviewed Mar 14, 2017

View reviewed changes

chore:some adjustment

23bcaa8

ngot merged commit e428f29 into master Mar 14, 2017

ngot deleted the refactor branch March 14, 2017 08:47

		@@ -0,0 +1,18 @@
		'use strict';

		const XTPL = Symbol('app#XTPL');

		@@ -0,0 +1,18 @@
		'use strict';

		const XTPLSYMBOL = Symbol('app#xtpl');

feat: refactor #2

feat: refactor #2

Conversation

ngot commented Mar 13, 2017 • edited

Checklist

Affected core subsystem(s)

Description of change

codecov bot commented Mar 13, 2017 • edited

Codecov Report

ngot commented Mar 13, 2017

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

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

ngot Mar 14, 2017 • edited

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

Choose a reason for hiding this comment

ngot commented Mar 14, 2017

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 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atian25 commented Mar 14, 2017

ngot commented Mar 14, 2017

atian25 commented Mar 14, 2017

ngot commented Mar 14, 2017

atian25 commented Mar 14, 2017

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 14, 2017

ngot commented Mar 14, 2017

ngot commented Mar 14, 2017

ngot commented Mar 13, 2017 •

edited

codecov bot commented Mar 13, 2017 •

edited

ngot Mar 14, 2017 •

edited

atian25 left a comment •

edited