feat: Provide `file` mode to handle multipart request #19

fengmk2 · 2018-09-28T07:22:27Z

see RFC to get detail: eggjs/egg#3052

package.json

codecov · 2018-09-28T12:26:07Z

Codecov Report

Merging #19 into master will decrease coverage by 1.89%.
The diff coverage is 97.08%.

@@           Coverage Diff            @@
##           master     #19     +/-   ##
========================================
- Coverage     100%   98.1%   -1.9%     
========================================
  Files           3       5      +2     
  Lines          56     158    +102     
========================================
+ Hits           56     155     +99     
- Misses          0       3      +3

Impacted Files	Coverage Δ
config/config.default.js	`100% <100%> (ø)`	⬆️
app.js	`100% <100%> (ø)`	⬆️
app/extend/context.js	`100% <100%> (ø)`	⬆️
app/schedule/clean_tmpdir.js	`93.33% <93.33%> (ø)`
app/middleware/multipart.js	`98.24% <98.24%> (ø)`

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 0b4e118...a9c9507. Read the comment docs.

deps on eggjs/egg-multipart#19

fengmk2 · 2018-09-28T13:54:44Z

示例 eggjs/examples#86

fengmk2 · 2018-09-28T14:00:15Z

app/schedule/cleanup_tmpdir.js

+const moment = require('moment');
+
+module.exports = app => {
+  return class CleanTmpdir extends (app.Subscription || app.BaseContextClass) {


eggjs/egg#3058

fengmk2 · 2018-09-28T15:39:14Z

app.js

+
+  options.mode = options.mode || 'stream';
+  if (![ 'stream', 'file' ].includes(options.mode)) {
+    throw new TypeError(`Expect mode to be 'stream' or 'file', but got '${options.mode}'`);


这里没有覆盖。

fengmk2 · 2018-09-28T15:43:20Z

app/middleware/multipart.js

+          return await limit(requestFiles, 'Request_fieldSize_limit', 'Reach fieldSize limit');
+        }
+        if (fieldnameTruncated) {
+          return await limit(requestFiles, 'Request_fieldNameSize_limit', 'Reach fieldNameSize limit');


busboy 目前不支持，所以无法覆盖。

see RFC to get detail: eggjs/egg#3052

deps on eggjs/egg-multipart#19

atian25 · 2018-09-29T01:05:40Z

README.md

+
+## `file` mode: the easy way
+
+If you don't know the [Node.js Stream](https://nodejs.org/dist/latest-v10.x/docs/api/stream.html) work, maybe you should use the `file` mode to get started.


This is the quickstart for Node newcomer who don't familiar with Node.js Stream .

You should not use this feature at big project, using Stream could avoid memory issues.

cc @Maledong help for the english

You should not use this feature at big project, using Stream could avoid memory issues.

未必的，使用文件也不会出问题。

速度会有影响吧，一个是边接收边送过去 oss，一个是先存到本地，再丢 oss

在语雀的真实用途中，如果真的要对文件进行处理，保存到磁盘会更快。传送到 oss 的速度还取决于用户到服务器的速度。其实两者差别不大，除非你的服务器与 oss 很慢。

也是，学习了

传送到 oss 的速度还取决于用户到服务器的速度。

这个其实 nginx 也会做，解决用户到服务器的问题

atian25 · 2018-09-29T01:07:50Z

README.md

+};
+```
+
+## `stream` mode: the hard way


the hard and recommended way

不推荐。。。我们也未必能写好。你看看中间件里面的异常处理代码。

app/middleware/multipart.js

app/schedule/clean_tmpdir.js

atian25 · 2018-09-29T01:23:26Z

package.json

-    "lint": "eslint .",
-    "test": "npm run lint -- --fix && npm run test-local",
+    "lint": "eslint . --fix",
+    "test": "npm run lint && npm run test-local",


should change to npm run lint -- --fix

atian25 · 2018-09-29T01:23:42Z

package.json

  },
  "description": "multipart plugin for egg",
  "main": "index.js",
  "scripts": {
    "autod": "autod",
-    "lint": "eslint .",
-    "test": "npm run lint -- --fix && npm run test-local",
+    "lint": "eslint . --fix",


this will make ci skip lint check

windows 下没有 --fix 会导致换行符错误。

git 配置问题？其他项目都不会在 ci 用 --fix 的，win 的单测都没问题

应该是 git 配置问题，加了 fix 等于没测

fengmk2 · 2018-09-29T06:43:13Z

package.json

@@ -13,6 +16,7 @@
    "test-local": "egg-bin test",
    "cov": "egg-bin cov",
    "ci": "egg-bin pkgfiles && npm run lint && npm run cov",
+    "ci-windows": "egg-bin pkgfiles && npm run lint -- --fix && npm run cov",


加个单独的配置给 azure-pipelines

--fix 可以去掉

atian25 · 2018-09-29T06:49:07Z

文档也要对应更新下

deps on eggjs/egg-multipart#19

atian25 · 2018-09-29T09:39:02Z

README.md

+
+## License
+
+[MIT](LICENSE)


这里导致 egg 文档编译出错了，要写全链

https://github.com/eggjs/egg-multipart/blob/master/LICENSE

cc @Maledong

其实不是编译错，是测试错

popomore · 2018-09-30T04:13:07Z

覆盖率下降了

popomore · 2018-09-30T04:16:31Z

README.md

+- `ctx.request.body`: Get all the multipart fields and values, except `file`.
+- `ctx.request.files`: Contains all `file` from the multipart request, it's an Array object.
+
+**WARNING: you should remove the temporary upload file after you use it**


需要自己删？

popomore · 2018-09-30T04:20:37Z

README.md

+      result = await ctx.oss.put(name, file.filepath);
+    } finally {
+      // need to remove the tmp file
+      await fs.unlink(file.filepath);


可以在这次请求完成后自动删除？

不行的，没法判断怎样才算请求完，开发者可以后台处理

后台处理就没办法了

popomore · 2018-09-30T04:35:06Z

app/middleware/multipart.js

+        throw err;
+      }
+
+      if (!part) break;


~~这里是指空文件吗？~~ 看来是没有字段，那是不是直接 while 会比较好。

popomore · 2018-09-30T04:51:39Z

app/middleware/multipart.js

+      if (!storedir) {
+        // ${tmpdir}/YYYY/MM/DD/HH
+        storedir = path.join(options.tmpdir, moment().format('YYYY/MM/DD/HH'));
+        const exists = await fs.exists(storedir);


每个请求都会走下 io、

只有有 file 才会走

popomore · 2018-09-30T04:52:23Z

app/middleware/multipart.js

+      }
+      const filepath = path.join(storedir, uuid.v4() + path.extname(meta.filename));
+      const target = fs.createWriteStream(filepath);
+      await pump(part, target);


这个报错不 clean 下？

这里可以优化

popomore · 2018-09-30T04:55:37Z

app/schedule/clean_tmpdir.js

+        } catch (err) {
+          ctx.coreLogger.error('[egg-multipart:CleanTmpdir:error] remove tmpdir: %j error: %s',
+            dir, err);
+          ctx.coreLogger.error(err);


直接改 err.message?

Due to eggjs/egg-multipart#19, we have to add this new feature into Egg's main doc in controller.md to notify clients.

docs (Controller.md): Add new feat description (#3066) Due to eggjs/egg-multipart#19, we have to add this new feature into Egg's main doc in controller.md to notify clients.

fengmk2 added the enhancement label Sep 28, 2018

atian25 reviewed Sep 28, 2018

View reviewed changes

package.json Show resolved Hide resolved

fengmk2 force-pushed the easy-mode branch from a48a8c8 to 1799862 Compare September 28, 2018 13:53

fengmk2 added a commit to eggjs/examples that referenced this pull request Sep 28, 2018

feat: add file mode example on multipart

3b2c4f0

deps on eggjs/egg-multipart#19

fengmk2 mentioned this pull request Sep 28, 2018

feat: add file mode example on multipart eggjs/examples#86

Merged

4 tasks

fengmk2 changed the title ~~[WIP] easy~~ feat: Provide file mode to handle multipart request Sep 28, 2018

fengmk2 commented Sep 28, 2018

View reviewed changes

feat: Provide file mode to handle multipart request

3c512f9

see RFC to get detail: eggjs/egg#3052

fengmk2 force-pushed the easy-mode branch from 165f1e9 to 3c512f9 Compare September 28, 2018 17:18

fengmk2 added a commit to eggjs/examples that referenced this pull request Sep 28, 2018

feat: add file mode example on multipart

9848cf0

deps on eggjs/egg-multipart#19

atian25 reviewed Sep 29, 2018

View reviewed changes

fengmk2 added 2 commits September 29, 2018 14:41

f

4624279

f

a9c9507

fengmk2 commented Sep 29, 2018

View reviewed changes

atian25 approved these changes Sep 29, 2018

View reviewed changes

fengmk2 merged commit 75c0733 into master Sep 29, 2018

fengmk2 deleted the easy-mode branch September 29, 2018 06:46

fengmk2 added a commit to eggjs/examples that referenced this pull request Sep 29, 2018

feat: add file mode example on multipart

14ab0ae

deps on eggjs/egg-multipart#19

fengmk2 added a commit to eggjs/examples that referenced this pull request Sep 29, 2018

feat: add file mode example on multipart (#86)

6a03a0b

deps on eggjs/egg-multipart#19

atian25 reviewed Sep 29, 2018

View reviewed changes

popomore reviewed Sep 30, 2018

View reviewed changes

ghost mentioned this pull request Sep 30, 2018

docs (Controller.md): Add new feat description eggjs/egg#3066

Merged

4 tasks

fengmk2 pushed a commit to eggjs/egg that referenced this pull request Sep 30, 2018

docs (Controller.md): Add new feat description (#3066)

0d6798d

Due to eggjs/egg-multipart#19, we have to add this new feature into Egg's main doc in controller.md to notify clients.

popomore pushed a commit to eggjs/egg that referenced this pull request Sep 30, 2018

docs: auto generate by ci

3d78b2f

docs (Controller.md): Add new feat description (#3066) Due to eggjs/egg-multipart#19, we have to add this new feature into Egg's main doc in controller.md to notify clients.


		## `file` mode: the easy way

		If you don't know the [Node.js Stream](https://nodejs.org/dist/latest-v10.x/docs/api/stream.html) work, maybe you should use the `file` mode to get started.


		## License

		[MIT](LICENSE)

feat: Provide file mode to handle multipart request #19

feat: Provide file mode to handle multipart request #19

Conversation

fengmk2 commented Sep 28, 2018 • edited Loading

codecov bot commented Sep 28, 2018 • edited Loading

Codecov Report

fengmk2 commented Sep 28, 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

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

atian25 commented Sep 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popomore commented Sep 30, 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

popomore Sep 30, 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

feat: Provide `file` mode to handle multipart request #19

feat: Provide `file` mode to handle multipart request #19

fengmk2 commented Sep 28, 2018 •

edited

Loading

codecov bot commented Sep 28, 2018 •

edited

Loading

popomore Sep 30, 2018 •

edited

Loading