Slack archive

cofacts

Month: 2020-05

2020-05-01

Nacs 10:36:15
@nacs13 has joined the channel
github 15:22:31

#183 GraphQL API enhancements

From comments in <https://github.com/cofacts/rumors-line-bot/pull/169|#169>: • `Query.context` is a little bit confusing with apollo context • `UserContext.state` &amp; `UserContext.data` should be non-nullable

github 15:34:11

*<https://github.com/cofacts/rumors-line-bot/compare/a28f1b6b5ba2...8a390abf8dac|8 new commits> pushed to <https://github.com/cofacts/rumors-line-bot/tree/dev|`dev`>* <https://github.com/cofacts/rumors-line-bot/commit/080040ad2857b3c1d410465f4ccb5a755f48caf7|`080040ad`> - GraphQL authentication &amp; context population <https://github.com/cofacts/rumors-line-bot/commit/30c8a3e52d0715a56eb6eace1484d834eb29cfe4|`30c8a3e5`> - Implement authentication, @auth directive and context field <https://github.com/cofacts/rumors-line-bot/commit/da8ccf6e33848f919e4459fbe91d3b7391052851|`da8ccf6e`> - Redundent jest.mock() as manual mocks for node modules will always load. <https://github.com/cofacts/rumors-line-bot/commit/f24191c880cf28a4fb879d64a1f440ea76071f14|`f24191c8`> - Add "manual" (auto) mock for redisClient so that all redisClient deps are mocked (such as in graphql/__tests__/insights) <https://github.com/cofacts/rumors-line-bot/commit/8dfa0c12afe6b0f6c8b49b798e4474c955450a89|`8dfa0c12`> - Prettier fix <https://github.com/cofacts/rumors-line-bot/commit/5295edb20990e761d51773539fab7903d37fc657|`5295edb2`> - Avoid calling the real redisClient in unit tests <https://github.com/cofacts/rumors-line-bot/commit/97b491de65d9a67c9c92bbdb4ac4118ff8f8359f|`97b491de`> - Add userId in GraphQL context after authentication <https://github.com/cofacts/rumors-line-bot/commit/8a390abf8dac39ad4701a9870628cbb1f06dad0c|`8a390abf`> - Merge pull request #169 from cofacts/auth-api

github 15:37:06

*<https://github.com/cofacts/rumors-line-bot/compare/8a390abf8dac...c5b684609691|9 new commits> pushed to <https://github.com/cofacts/rumors-line-bot/tree/dev|`dev`>* <https://github.com/cofacts/rumors-line-bot/commit/d36d76e47d90f391580d4ca5866aab14aa1efcf5|`d36d76e4`> - Implements mutation GraphQL endpoints for LIFF <https://github.com/cofacts/rumors-line-bot/commit/deb8bdd8075e5107db100ca2062caff5d5dccaba|`deb8bdd8`> - Remove submitReplyRequest because it is no longer needed <https://github.com/cofacts/rumors-line-bot/commit/ea856c552b8e8612936609b8fec6773c7a53d31d|`ea856c55`> - Remove submitArticle mutation because it's not needed <https://github.com/cofacts/rumors-line-bot/commit/b853bed252645822f9a81eccd79a9b72160efb64|`b853bed2`> - Prettier fix <https://github.com/cofacts/rumors-line-bot/commit/7860ec7dcc8c773ee25755d1563e26a3a4d16ac1|`7860ec7d`> - Add unit test for gql <https://github.com/cofacts/rumors-line-bot/commit/8ec314510a5e31c4de95248b3e6b09caa690a9b6|`8ec31451`> - Lint fix <https://github.com/cofacts/rumors-line-bot/commit/cc864507472181a1d2d713d18e5ecb675ac41824|`cc864507`> - gql error handling test. For test coverage... <https://github.com/cofacts/rumors-line-bot/commit/f86275a983ddd303333466dea15de103e17159ca|`f86275a9`> - Prettier fix <https://github.com/cofacts/rumors-line-bot/commit/c5b684609691bad8ec9ed6bd83df397dae310f40|`c5b68460`> - Merge pull request #170 from cofacts/mutation-api

github 15:37:22

Successfully deployed <https://github.com/cofacts/rumors-line-bot/commit/8a390abf8dac39ad4701a9870628cbb1f06dad0c|`8a390ab`> to <https://dashboard.heroku.com/apps/rumors-line-bot-staging/activity/builds/d14cc973-eef1-4425-9095-7c854349ac09|rumors-line-bot-staging>

github 15:38:01

*<https://github.com/cofacts/rumors-line-bot/compare/c5b684609691...8cc7ede5ddaf|8 new commits> pushed to <https://github.com/cofacts/rumors-line-bot/tree/dev|`dev`>* <https://github.com/cofacts/rumors-line-bot/commit/9d7f9b991f55c65a5f282ef0e11f1e0307099b1a|`9d7f9b99`> - Setup svelte liff and server build <https://github.com/cofacts/rumors-line-bot/commit/0bb6c769cdefba3d5d1589579d3fc3096c2779b7|`0bb6c769`> - audit fix <https://github.com/cofacts/rumors-line-bot/commit/51673df03e493ef42ddc49abbd3cfee1789d1396|`51673df0`> - Add liff proxy in dev mode <https://github.com/cofacts/rumors-line-bot/commit/eb8d1a7859035ff2b936544b29521d590465fbc6|`eb8d1a78`> - Add basic svelte setup <https://github.com/cofacts/rumors-line-bot/commit/0c2d09f027979d223b13077a61796da77fdc49ef|`0c2d09f0`> - Svelte i18n and corejs polyfill <https://github.com/cofacts/rumors-line-bot/commit/711fe7f2fb924332b0d94991777211c2fe3a1bfb|`711fe7f2`> - Imports polyfills that supports Safari10 and Android 52 <https://github.com/cofacts/rumors-line-bot/commit/10bb13afcf58cda3e5a09a68f61b13df3aeecd28|`10bb13af`> - Eslint ignore built liff/* <https://github.com/cofacts/rumors-line-bot/commit/8cc7ede5ddaf463e80977208ae97a93c81fa68f5|`8cc7ede5`> - Merge pull request #171 from cofacts/svelte-liff

mrorz 19:34:45
/github unsubscribe cofacts commits,statuses,deployments,releases
github 19:34:45

This channel will get notifications from <https://github.com/cofacts|cofacts> for: `issues`, `pulls`, `public`

mrorz 19:35:18
/github subscribe cofacts reviews,comments
github 19:35:18

This channel will get notifications from <https://github.com/cofacts|cofacts> for: `issues`, `pulls`, `public`, `comments`, `reviews`

mrorz 19:37:50
@ggm PR review 囉，請過目
https://github.com/cofacts/rumors-line-bot/pull/182

mongoClient 的實作我覺得有些太複雜了，abstraction 也有點怪 QQ

#182 Feature/162 setup db

To see <https://github.com/cofacts/rumors-line-bot/issues/162|#162>

噢其實我覺得好像繼承 mongoClient 比較好
然後為什麼不要用 es6 class 呀
其他我晚點繼續看噢
React 寫久就會不喜歡 class (欸

其實主要是看到底想做的是 generalized client 還是 specialized singleton

我不反對做成 generalized client，就是包成 class、new 時進行連線，constructor 裡面設定 URL 等等

我也不反對另外做個 factory function 來吐一個前述的 class 的 singleton instance

但現在這個 class 實在太詭異，輸出的 constructor 可以改 URL 卻不能改 dbName，又有 singleton 的 static method、也會自動 connect 等等
我們對 `mongoClient` 還有下面的需求，會讓我覺得要滿足這些需求時，做成 class 反而是比較多餘的封裝

1. 我們會希望整個 app 共用一個 MongoDB 連線，而不是每個 request 來就重新連 MongoDB
2. 我們會連的資料庫就是 env vars 裡面指定的那些，不會有別的資料庫
這應該也是為什麼你會設計 `getInstance` 這個 static method 來回傳 singleton，因為 singleton 才是能滿足我們需求的 pattern

但如果我們想要的是 singleton 的話，那在 Javascript 的世界裡，我們大可以直接使用 object literal 就能辦到，不需要大費周章地包成 class 然後 instantiate 自己之類的
我是覺得自動連線的地方比較怪異沒錯，但其實你改成的 `getClient()` 的方式也是做了一樣的事情？「可以改 URI 卻不能改 dbName」為什麼不合理呀？我們不會用到別的 db 所以 `dbName` 不能改，但是會根據 `production` 和 `staging` 來有不同的 URI，所以 MONGODB_URI 會透過環境變數修改
主要的差別在 expose 出整個 class，跟只吐一個 factory method 的差別唷
應該是說我在裡面寫的那份 code 就是
「可以達成一樣的事情，但不會 leak 其他邏輯（規定要某種 call 法）的 abstraction」
嗯懂
但還是有隱含已連線的問題，要真的解決這個怪異，可能改成 getConnectedClient 之類的吧
然後我不覺得這是大費周章的包成 class 耶，不過就是定義一個 class 寫個 singleton ，用 object literal 的寫法不就也是模擬成 class 的樣子嗎？（吐出一個 object，然後他有一些 function 可以呼叫）那為什麼不直接用 class 來定義勒？
`getConnectedClient` 比較像是吐 object 的 factory (function) 的感覺
還是你是想說的是，既然我們都可以用 object literal 來做了，何須用 class
恩也可以這麼說
試想我們今天如果要加一上另外一個 function，你要做的事情是在 setupClient 的回傳值裡面加入一個 function
而 class 方法要做的事情，是在 class 裡面加入一個 function
我覺得 class 的做法不是比較直覺比較合理嗎？
我去找到這個 class 定義在哪裡，替他加上一個 function
我的直覺會是
我找到那個物件在哪裡，然後加上一個 method
而且 object literal 的做法，就會讓回傳值包得長長的
因為我要一包物件
他出生就是個 instance
factory 只是因為我想把連線之類的事情，從 import file 的時機點，延後到呼叫 `getClient`時做
不然其實我覺得 import 時就 connect 其實也挺好的
這樣就真的只要 `export default obj` 然後那個 `obj` 就是整個 module 的內容惹
啊其實不太行，最後至少還是要回傳個 promise
要等 connect (async) 之後才能讓使用者操作
嗯一定要回 promise
不然就是在某個地方先 await 過之類的（感覺很奇怪）
那就是 promise of 那個可以 db 呀 collection 呀 close 呀的 API （一個物件）
畢竟 JS 的 module.exports 本身就是個大 singleton
那我好奇噢，如果之後轉 typescript 的話，class 的做法可以明確的知道定義長怎樣，不管是每個 function 的或是自己，那這樣的話 object literal 的做法會長怎樣？
我需要另外定義一個 Type/Interface 來描述他是嗎？
轉 Typescript 的話
我會訂個 type 描述 `CofactsMongoClient`

然後定 getClient 會回傳 `Promise<CofactsMongoClient>`
那些 function 的 type 都指定好之後，讓 typescript 的 structural typing 確認我的 implementation 是不是確實會生出這個形狀的東西
嗯但 class 的做法不用另外再描述
對外面來說他就是吐 `Promise<CofactsMongoClient>` 的 function~
如果要 class 的話，那我覺得我們就在 constructor 裡面回傳那個 singleton instance 吧

像是這份答案的 ES7 version (Update 2019) https://stackoverflow.com/a/6733919/1582110

把 class expose 出去的時候，外面的人只能做一件事情，就是 new 它
然後 new 它，就只會回傳那個 singleton，然後連線啥的都會自動做好這樣

這樣一來外面的使用者只有一種使用方法，會是比較合適的封裝
就是 constructor 裡面會處理 getInstance or getClient 的概念
（我發現另一篇類似發問文的 best answer 就是 instance XD 但那是在 class 出現之前寫的）

https://stackoverflow.com/questions/1479319/simplest-cleanest-way-to-implement-singleton-in-javascript
好我來看看，然後把 connected 寫在註解好了，我覺得拿到一個已連線的 client 真的怪怪的 XDD 但每次呼叫 .connect() 也不是辦法
對啊沒有 class 一定是會這樣寫呀 還是 var 耶
我覺得比較棘手的部分是，我們的東西是 async 的，所以 class constructor 要怎麼處理那個 async 的部分其實滿討厭
`Downvote for the accepted answer not being a singleton at all. It's just a global variable. – mlibby Feb 25 '13 at 12:53`
XDD
對沒有原生 class 的 JS 來說
真的就是 global variable 呀 XD
對啊他好嚴格
大概是四人幫的粉絲、喜歡 Java 不喜歡 Javascript 的人 (?
> class constructor 要怎麼處理那個 async 的部分其實滿討厭
我現在想到的東西超怪的耶，就是 constructor 會回傳一個 promise，然後 promise 會 resolve to singleton instance when connected to database

所以會有一個超詭異的狀況：

我 new 一個 class ，但拿到的不是該 class 的 instance 而是 promise of that instance

超反直覺的
除非我們 extend Promise (更鬧了)
要不然就是保留現在的 getInstance

未來 Typescript 的時候直接用 private constructor 把 new 那條路堵起來，這樣只能呼叫 `getInstance` 抓 instance

https://khalilstemmler.com/blogs/typescript/when-to-use-a-private-constructor/
extend Promise 很帥耶⋯
我不太能接受 extend Promise

我覺得 private constructor 是最接近你原本想做的東西了
現在沒有 typescript
就放個註解說「不要 new!」吧
嗯 typescript 的話就可以 private constructor
好
啊然後其實 dbName 也可以拿掉，用預設的，也就是 MONGODB_URI 的
喔喔那個 mongodb connection uri `/` 後面帶的 db name 是 auth 用 db，不一定等於資料存放的 db 的說
`/` 後面是 dbname 沒錯， `authsource` 可以指定另外的 auth
欸不是
所以就算有帶 `/dbName` 我們還是要 `db(dbName)` 一次
我要說的是可以這樣 `/cofacts?authsource=admin`
嗯但這樣連線完，你似乎還是得要 `.db('cofacts')` ?
可以試試看
我的印象是還是要 `db` 指定 db name
不用噢
The name of the database we want to use. If not provided, use database name from connection string.
真的耶
學到了
酷
ggm 19:51:11
好啊我看看

2020-05-02

github 00:25:15

Comment on #182 Feature/162 setup db

Update: as <https://g0v-slack-archive.g0v.ronny.tw/index/channel/C2PPMRQGP/2020-05|discussed in Slack> we decided to add a comment to constructor to indicate that the users of `CofactsMongoClient` should never call `new` by themselves. When we adopt Typescript in the future, we will make the constructor private to make sure that users of `CofactsMongoClient` can only retrieve its singleton instance via `getInstance()` static method. `CofactsMongoClient` is dedicated to connecting to Cofacts LINE bot mongodb, not something that is generalized. ES6 class is used so that it's easier to define method and update the client type in the same time, also ensures a smoother updater when we rewrite in Typescript.

mrorz 01:11:54
這份在 review 中的 UI，目前放上 staging 囉
https://cofacts.hacktabl.org/

其中 reply list 會遇到之前 user 是 null 的問題

cofacts.hacktabl.org

Cofacts - Connecting facts and instant messages

Cofacts is a collaborative system connecting instant messages and fact-check reports or different opinions together. It’s a grass-root effort fighting mis/disinformation in Taiwan.