Feature/refactor server #289

just-at-uber · 2021-04-23T17:07:35Z

This PR has no functional changes, only syntax / readability changes to help future work with ES support. Since a lot of files have been created to split apart helpers into single files, each file needs a license header which adds to the lines-of-code added.

Changed

moved helper functions into separate files for better readability
moved require (imports) to top of files to match import compatibility in the future
moved most constants into separate constants file

just-at-uber · 2021-04-23T17:10:25Z

server/middleware/tchannel-client.js

@@ -1,304 +0,0 @@
-// Copyright (c) 2017-2021 Uber Technologies Inc.


file moved to server/middleware/tchannel-client/index.js

just-at-uber · 2021-04-23T17:11:56Z

server/helpers/index.js

+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+const momentToLong = require('./moment-to-long');


moved helper functions into separate files for better readability

just-at-uber · 2021-04-23T17:12:08Z

server/helpers/moment-to-long.js

+
+const Long = require('long');
+
+const momentToLong = m => Long.fromValue(m.unix()).mul(1000000000);


moved helper functions into separate files for better readability

just-at-uber · 2021-04-23T17:13:14Z

server/index.js

-app.webpackConfig = require('../webpack.config');
+const webpackConfig = require('../webpack.config');
+const tchannelClient = require('./middleware/tchannel-client');
+const router = require('./router');


moved require (imports) to top of file to match import compatibility in the future

just-at-uber · 2021-04-23T17:15:32Z

server/index.js

    .use(
-      require('koa-compress')({


moved require (imports) to top of files to match import compatibility in the future

just-at-uber · 2021-04-23T17:15:40Z

server/index.js

        filter: contentType => !contentType.startsWith('text/event-stream'),
      })
    )
-    .use(require('./middleware/tchannel-client'))


moved require (imports) to top of files to match import compatibility in the future

just-at-uber · 2021-04-23T17:15:48Z

server/index.js

    .use(
      useWebpack
        ? koaWebpack({
            compiler,
            dev: { stats: { colors: true } },
            hot: { port: process.env.TEST_RUN ? 8082 : 8081 },
          })
-        : require('koa-static')(staticRoot)


moved require (imports) to top of files to match import compatibility in the future

just-at-uber · 2021-04-23T17:16:01Z

server/index.js

  let compiler;

  if (useWebpack) {
-    const Webpack = require('webpack');
-
-    koaWebpack = require('koa-webpack');


moved require (imports) to top of files to match import compatibility in the future

just-at-uber · 2021-04-23T17:16:54Z

server/middleware/tchannel-client/constants.js

+  timeout: REQUEST_TIMEOUT,
+  retryFlags: REQUEST_RETRY_FLAGS,
+  retryLimit: REQUEST_RETRY_LIMIT,
+};


moved most constants into separate constants file

just-at-uber · 2021-04-23T17:17:16Z

server/middleware/tchannel-client/helpers/cli-transform.js

+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.


moved helper functions into separate files for better readability

just-at-uber · 2021-04-23T17:17:31Z

server/middleware/tchannel-client/helpers/format-body.js

+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.


moved helper functions into separate files for better readability

just-at-uber · 2021-04-23T17:17:41Z

server/middleware/tchannel-client/helpers/format-method.js

+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.


moved helper functions into separate files for better readability

just-at-uber · 2021-04-23T17:17:49Z

server/middleware/tchannel-client/helpers/format-request-name.js

+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.


moved helper functions into separate files for better readability

just-at-uber · 2021-04-23T17:18:01Z

server/middleware/tchannel-client/helpers/index.js

+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.


moved helper functions into separate files for better readability

just-at-uber · 2021-04-23T17:18:09Z

server/middleware/tchannel-client/helpers/lookup-async.js

+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.


moved helper functions into separate files for better readability

just-at-uber · 2021-04-23T17:18:17Z

server/middleware/tchannel-client/helpers/make-channel.js

+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.


moved helper functions into separate files for better readability

just-at-uber · 2021-04-23T17:19:16Z

server/middleware/tchannel-client/helpers/make-request.js

+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.


moved helper functions into separate files for better readability

just-at-uber · 2021-04-23T17:19:38Z

server/middleware/tchannel-client/helpers/ui-transform.js

+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.


moved helper functions into separate files for better readability

just-at-uber · 2021-04-23T17:19:48Z

server/middleware/tchannel-client/helpers/with-domain-paging-and-workflow-execution.js

+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.


moved helper functions into separate files for better readability

just-at-uber · 2021-04-23T17:19:57Z

server/middleware/tchannel-client/helpers/with-domain-paging.js

+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.


moved helper functions into separate files for better readability

just-at-uber · 2021-04-23T17:20:05Z

server/middleware/tchannel-client/helpers/with-verbose-workflow-execution.js

+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.


moved helper functions into separate files for better readability

just-at-uber · 2021-04-23T17:20:13Z

server/middleware/tchannel-client/helpers/with-workflow-execution.js

+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.


moved helper functions into separate files for better readability

just-at-uber · 2021-04-23T17:20:23Z

server/middleware/tchannel-client/index.js

+  withWorkflowExecution,
+  withVerboseWorkflowExecution,
+  withDomainPagingAndWorkflowExecution,
+} = require('./helpers');


moved helper functions into separate files for better readability

just-at-uber · 2021-04-23T17:21:59Z

server/middleware/tchannel-client/index.js

@@ -0,0 +1,128 @@
+// Copyright (c) 2017-2021 Uber Technologies Inc.


moved from server/middleware/tchannel-client.js

just-at-uber · 2021-04-23T17:22:43Z

server/router.js

+const moment = require('moment');
+
+const featureFlags = require('./feature-flags.json');
+const { momentToLong } = require('./helpers');


moved helper functions into separate files for better readability

vytautas-karpavicius · 2021-04-23T17:43:52Z

server/index.js

-  app = new Koa(),
-  router = require('./routes');
+const path = require('path');
+const Koa = require('koa');


Shouldn't this be lowecase like the rest of consts here?

i'll update. casing ultimately has no difference in javascript but its good to keep to convention.

Looking at the code more closely, the code actually calls new on Koa. this would indicate that this is a class (or constructor). Typically in javascript we notate classes vs functions by using first capital letter. So i think in this particular case the casing is correct.

You can look at this convention here:
https://stackoverflow.com/questions/1564398/javascript-method-naming-lowercase-vs-uppercase/1564489#:~:text=5%20Answers&text=A%20popular%20convention%20in%20Javascript,mistakenly%20called%20%22classes%22).&text=Anything%20that's%20not%20a%20constructor%20usually%20starts%20with%20lowercase%20and%20is%20camelCased.

vytautas-karpavicius · 2021-04-23T17:55:30Z

server/middleware/tchannel-client/helpers/make-channel.js

+
+  const tchannelAsThrift = TChannelAsThrift({
+    channel: cadenceChannel,
+    entryPoint: path.join(__dirname, '../../../idl/cadence.thrift'),


This has two level upwards path (../..) compared to previous change. Is this intentional?

its because its path has now moved two levels deeper from where the code was located earlier.
server/middleware/tchannel-client.js => server/middleware/tchannel-client/helpers/make-channel.js

vytautas-karpavicius · 2021-04-23T18:00:47Z

server/middleware/tchannel-client/helpers/with-domain-paging.js

+
+const get = require('lodash.get');
+
+const withDomainPaging = ctx => body => {


Just curiuos, how should ctx => body =>be read? Is lambda to lambda? I assume two arguments would be specified as (ctx, body) =>

currying functions, so a function returns a function waiting to be executed before proceeding.
In this case ctx is prepped into scope as previously this was just accessed from outside the function. body onwards will be executed by koa server when the route is called and will pass the body automatically when calling the request handler.
Sorry if this isn't clear enough.

some articles on currying in javascript:
https://javascript.info/currying-partials
https://blog.benestudio.co/currying-in-javascript-es6-540d2ad09400

you can compare the old version of the function within the removed code server/middleware/tchannel-client.js for more context.

Ahh, currying, nice functional stuff :)

mkolodezny

LGTM

just-at-uber added 3 commits April 22, 2021 16:47

refactoring server

b2457b9

more refactoring

aec6f41

fix lint

896cb5a

just-at-uber commented Apr 23, 2021

View reviewed changes

just-at-uber marked this pull request as ready for review April 23, 2021 17:27

just-at-uber requested a review from a team April 23, 2021 17:27

vytautas-karpavicius approved these changes Apr 23, 2021

View reviewed changes

mkolodezny approved these changes Apr 23, 2021

View reviewed changes

just-at-uber merged commit ebb847d into master Apr 23, 2021

just-at-uber deleted the feature/refactor-server branch April 23, 2021 18:28

just-at-uber mentioned this pull request May 27, 2021

v3.26.0 #332

Merged

		@@ -1,304 +0,0 @@
		// Copyright (c) 2017-2021 Uber Technologies Inc.


		const Long = require('long');

		const momentToLong = m => Long.fromValue(m.unix()).mul(1000000000);

		@@ -0,0 +1,128 @@
		// Copyright (c) 2017-2021 Uber Technologies Inc.


		const get = require('lodash.get');

		const withDomainPaging = ctx => body => {

Feature/refactor server #289

Feature/refactor server #289

Conversation

just-at-uber commented Apr 23, 2021 • edited Loading

Changed

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

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

just-at-uber Apr 23, 2021 • 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

Choose a reason for hiding this comment

mkolodezny left a comment

Choose a reason for hiding this comment

just-at-uber commented Apr 23, 2021 •

edited

Loading

just-at-uber Apr 23, 2021 •

edited

Loading