> On Oct. 4, 2017, 9:42 p.m., Stephan Erb wrote: > > Regarding the question nodejs vs js thrift: nodejs supports both binary > > (good) and compact (better) thrift protocols. This should reduce scheduler > > and client side serialization overhead and lead to a slightly more snappy > > UI. > > David McLaughlin wrote: > But it doesn't support XMLHttpRequest, so it can't be used in a UI bundle.
Or maybe I misunderstood the comparison (between nodejs and js thrift)? To try and give a clearer picture of the problems: ThriftJS library: absolutely required to deserialize the Thrift over HTTP API the Scheduler exposes (unless we pin the UI to /apibeta). The files generated by the Thrift compiler (e.g. api_types.js, ReadOnlyScheduler.js) are also required in order to build query objects (like TaskQuery, etc.). So when you generate the files, you have two options: node, js or jquery. Node: * Supports CommonJS modules, meaning you don't have to rely on global scope and can instead import all Thrift types into your ES6 code (allowing you to reuse Thrift types in the app and your test code). * Assumes server-side environment, so uses the nodejs socket library to perform client requests, rather than XMLHttpRequest. jQuery: * Not only does it not support CommonJS, but the generated output doesn't even use var declarations for the variables. This caues it to break strict mode and means you can't even use node.vm to slurp it into the global context in test-setup.js. * Uses jQuery to perform all network requests, which uses the browser environment. JS: * As jQuery, but all web requests are synchronous. I tried an approach where I generated both jquery and node thrift files and then imported them only for tests. But the compiled code also assumes the ThriftJS library is also in the same format.. so it broke the tests and/or UI bundle. At which point I just attached the copy and pasted structs and enums to global in my test setup :) - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62720/#review187132 ----------------------------------------------------------- On Oct. 4, 2017, 5:43 a.m., David McLaughlin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62720/ > ----------------------------------------------------------- > > (Updated Oct. 4, 2017, 5:43 a.m.) > > > Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham. > > > Repository: aurora > > > Description > ------- > > Instance page moved over to the new UI. > > One of the big issues I ran into here was dealing with Thrift types. > Currently the Thrift compiler generates a file for use in JS. The compiler > gives you two options for JavaScript - one is 'node' which plays well with > our module system but assumes you'll be using the built-in socket > implementation in node, and then one for 'jquery' which uses the global > namespace but doesn't use vars (so we can't eval as it breaks strict mode). > So the TL;DR is - to write tests against scheduler states, I've had to copy > and paste the Thrift enums into a test setup file. > > The alternative is to simply ignore Thrift types and duplicate it all without > relying on globals. Thus making sure the tests and application code are using > the same enums. However, this approach won't help you find any UI regressions > when you update Thrift - and even worse, you won't spot any API regressions > when developing the UI. So I've just gone for this approach for now. > > > Diffs > ----- > > ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 > ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a > ui/src/main/js/components/InstanceHistory.js PRE-CREATION > ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION > ui/src/main/js/components/Layout.js > 4ca54e318786859e23dc0444d7d2750817428e81 > ui/src/main/js/components/StateMachine.js PRE-CREATION > ui/src/main/js/components/TaskDetails.js PRE-CREATION > ui/src/main/js/components/TaskStatus.js PRE-CREATION > ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION > ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js > PRE-CREATION > ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION > ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 > ui/src/main/js/pages/Instance.js PRE-CREATION > ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION > ui/src/main/js/test-utils/Builder.js PRE-CREATION > ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION > ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION > ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 > ui/src/main/js/utils/Task.js PRE-CREATION > ui/src/main/js/utils/Thrift.js PRE-CREATION > ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d > ui/src/main/sass/components/_instance-page.scss PRE-CREATION > ui/src/main/sass/components/_job-list-page.scss > d31344d8eabb41114f4ff4678ff841119dfdac82 > ui/src/main/sass/components/_layout.scss > 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 > ui/src/main/sass/components/_state-machine.scss PRE-CREATION > ui/src/main/sass/components/_status.scss PRE-CREATION > ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 > > > Diff: https://reviews.apache.org/r/62720/diff/2/ > > > Testing > ------- > > ./gradlew ui:lint > ./gradlew ui:test > > Testing in Vagrant. > > > File Attachments > ---------------- > > Active Task with State Machine component > > https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png > Expanded Finished Task > > https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png > Tooltip > > https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3-48a3-4292-8179-7d09bfbcc1e4__Screen_Shot_2017-10-03_at_10.28.33_PM.png > > > Thanks, > > David McLaughlin > >
