Skip to content
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

[WIP] feat: ファイル書き出しのサポート #15

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

latica-jp
Copy link
Collaborator

@latica-jp latica-jp commented Aug 6, 2019

  • ファイル書き出しのサポート(作業中)
  • ひとまずダウンロードパスを指定した書き出し操作は実装できた
    • ダウンロードしたら一覧から削除([x] ボタンをクリック)できるオプションがあるとよいかも
  • D&D による書き出し項目の追加について、基本は実装できた
    • ひとまず項目の一番左に追加するようにしてみた
      • どの項目のあとに追加するか指定できるとよい?
    • フィールドコードではなく、ラベルによる指定なので、同一名称が存在しうる
      • 同一名称のどちらかを指定するオプションが必要
  • 書き出しオプション(文字コード、区切り文字)の指定は未実装

await page.mouse.move(x, y)
await page.mouse.down()
await page.waitFor(100)
await page.mouse.move(columnsViewBox.x + 5, y)
Copy link
Collaborator

Choose a reason for hiding this comment

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

おおお。。。すごい、苦労が垣間見れるメソッドだ。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

いやー、もう苦労しました…と言いたいところですが、けっこう単純でした 😄

@@ -10,6 +10,11 @@ const getIndexUrl = (domain, appId, options = {}) => {
return `https://${domain}/k/${appId}/${queryString}`
}

const getDownloadFileUrl = (domain, appId, options = {}) => {
const queryString = toQueryString({ view: 20 })
return `https://${domain}/k/${appId}/exportRecord${queryString}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

optionsをシカトしているのが気になる。
viewの指定なしでexportRecordを開くとデフォルトビューの内容でカラムが選択されるので、
options無しならデフォルトビューの内容でexportでもいい気がする。
指定されたらそのviewで出力する感じで。

Copy link
Collaborator

Choose a reason for hiding this comment

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

デフォルトビュー→ 一覧を表示したときデフォルトで表示されるビュー ≠ view:20
※念の為

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ここ、ちょっとまだ暫定なのですが、ご指摘のとおりと思います。
view=id つけなければ、デフォルトビューになるんですね。
あと、メソッド名は exportFileUrl にしたほうがいいかな、と思いました。
「書き出し」ですもんね。

Copy link
Collaborator

@t-kojima t-kojima Aug 7, 2019

Choose a reason for hiding this comment

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

あ、でも対となる処理にupload.jsがあるのでdownloadのままでもよいかも

もしexportにするならあちらもimportにして貰えると 🙏

return fileName
}

const waitFileDownloadAndGetName = async downloadPath => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

【割と感覚的な名前の話】

  • メソッド名にwaitがあると、待つ以外の処理をしていると想像し辛い
  • xxx and xxx というメソッド名は2つの処理をしていることを示唆しているので、できれば避けたい
    • 一つのメソッドでは一つの処理のみを行う
  • getDownloadedFileName とかでも良いのかなと思う
    • ある程度時間がかかる(=waitする)のはawaitからも分かるので

とはいえこのファイルでしか参照されないローカルな関数なので、そこまで気にしなくてよいのかも

Copy link
Collaborator Author

@latica-jp latica-jp Aug 7, 2019

Choose a reason for hiding this comment

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

なるほど…

「指定したパスにファイルがダウンロードされてくるのを監視して、完了を検知したらファイル名を返す」という処理を端的に表すと…

wait より、もっと具体的に monitor とか detect とかかな。

detectFileDownload とか。

https://www.google.com/search?ei=UE5KXa-lEqGKr7wPo_m8wA8&q=detect+File+Download&oq=detect+File+Download

detect + file + download でぐぐると
そのものの記事がけっこう引っかかるところをみると、英語的にはありな感じかも。

動詞が「検知」なので、ファイルハンドルとか、ファイル名とか返すのは自然だと考えれば、-getName は不要かもですね。

Copy link
Collaborator

@t-kojima t-kojima Aug 7, 2019

Choose a reason for hiding this comment

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

detectFileDownload とか。

良いと思います 👍

const util = require('util')
const path = require('path')

const downloadFileAndGetName = async (page, downloadPath, downloadSelector) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

downloadFileというメソッド名で戻り値がファイル名でもそんなに違和感がないので、AndGetNameはなくても良いかも

欲を言うとgetDownloadFileNamedownloadFileに分けて、「ファイル名を取得する」「ダウンロードする」という処理でメソッドを分けるととてもしっくりくる。

Copy link
Collaborator Author

@latica-jp latica-jp Aug 7, 2019

Choose a reason for hiding this comment

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

たしかに、andGetName なくてよさそうですね!

こちら、前述のものと同じく、書き出しなので download を export に直そうかと思います。とすると、

exportFile = async (page, exportSelector, downloadPath) かな?と。

処理順を考えるとセレクタが前で、パスはあとかな…

exportPath ではなくて downloadPath なのは、kintone での「書き出し」、ブラウザでの「ダウンロード」を区別している感じです。

const selector = '#fm-field-toolitems-cybozu .fm-toolitem-gaia'
const index = await page.evaluate(
({ columnLabel, selector }) => {
const nodes = [...document.querySelectorAll(selector)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants