Details

    Attachments

      Activity

        Code should report 0 warnings and errors when linted with JSHint. Also, particularly important for un-compiled languages, everything needs a test (100% code coverage).

        I think that is all we need.

        codesf Randy Abernethy added a comment - Code should report 0 warnings and errors when linted with JSHint. Also, particularly important for un-compiled languages, everything needs a test (100% code coverage). I think that is all we need.

        There are other things that could be improved:

        (1) No leaking globals. Right now, the browser client leaks all the types into the global namespace. Use a module pattern CommonJS or UMD are both acceptable.
        (2) Standard callback approach - The NodeJS client uses the defacto callback approach where the error is the first argument and the result is the second argument. The browser client does not follow this standard at all (pretty much any serious browser app is either a browserify or requirejs app that rely on npm modules (even projects with bower usually also have package.json files for npm modules))
        (3) Code sharing between the browser javascript and nodejs javascript. The fact that both of these are maintained separately is strange, since there is a lot of code and tests that are the same between the two implementations.

        andrewdeandrade Andrew de Andrade added a comment - There are other things that could be improved: (1) No leaking globals. Right now, the browser client leaks all the types into the global namespace. Use a module pattern CommonJS or UMD are both acceptable. (2) Standard callback approach - The NodeJS client uses the defacto callback approach where the error is the first argument and the result is the second argument. The browser client does not follow this standard at all (pretty much any serious browser app is either a browserify or requirejs app that rely on npm modules (even projects with bower usually also have package.json files for npm modules)) (3) Code sharing between the browser javascript and nodejs javascript. The fact that both of these are maintained separately is strange, since there is a lot of code and tests that are the same between the two implementations.

        Hey Andrew, great points all. Overall Thrift consistency is really lacking in the both JS libs. Patches welcome!

        codesf Randy Abernethy added a comment - Hey Andrew, great points all. Overall Thrift consistency is really lacking in the both JS libs. Patches welcome!

        Randy,

        After all my other patches are in (because enforcing and updating the coding standards are going to change a LOT of lines of code and render all those patches stale), I'm going to add an .editorconfig file to lib/nodejs and add the lint-trap module to the devDependencies. lint-trap is a zero-config linter that has rules that raynos (one of the top contributors in the nodejs ecosystem) and I came up with. It enforces a lot of best practices and is designed for minimizing personal style (which is good for codebases with many contributors). Alternatively, I can just pull out the .eslintrc and .jscsrc files from lint-trap and commit them to thrift and use eslint and jscs instead.

        andrewdeandrade Andrew de Andrade added a comment - Randy, After all my other patches are in (because enforcing and updating the coding standards are going to change a LOT of lines of code and render all those patches stale), I'm going to add an .editorconfig file to lib/nodejs and add the lint-trap module to the devDependencies. lint-trap is a zero-config linter that has rules that raynos (one of the top contributors in the nodejs ecosystem) and I came up with. It enforces a lot of best practices and is designed for minimizing personal style (which is good for codebases with many contributors). Alternatively, I can just pull out the .eslintrc and .jscsrc files from lint-trap and commit them to thrift and use eslint and jscs instead.
        roger Roger Meier added a comment -

        eslint and jscs look good, but what is the reason for using both. Why not jscs only?
        What about the js lib? What fits best there?

        roger Roger Meier added a comment - eslint and jscs look good, but what is the reason for using both. Why not jscs only? What about the js lib? What fits best there?

        If we were to only use one, 100% go with eslint. It's way better than all the other linters out there. The reason for using both is that jscs has more whitespace rules than eslint. However, eslint is catching up on the whitespace rules that jscs has.

        The rules we used in lint-trap were designed isomorphic code in the npm ecosystem. i.e. any code that needs to work in both nodejs and in the browser via browserify.

        Some of the rules would not work with the code in lib/js. For example, the lib/js stuff relies on globals on the window object, which is something that we expressly forbid in rules we came up with. My personal opinion is that the code in lib/js and the generator for lib/js should be deprecated in favor of a single codebase in lib/nodejs. From that single codebase, we can use browserify to generate a "standalone" version of thrift (see the standalone documentation in the browserify readme https://github.com/substack/node-browserify ). The only additional work that would be required would be to take the generated nodejs client library and put it in a wrapper that exposes the same globals the client library currently exposes (since the nodejs library doesn't pollute the global namespace at all.)

        I can't think of a solid reason, except backwards compatibility (API) for maintaining both a lib/js and lib/nodejs. With a little work and reorganization, everything common between the browser and nodejs should be shareable. The browser.js file in THRIFT-2976 is my first attempt at a file that could be browserified with the standalone option.

        If I were to design the lib/js generator from scratch, I would have the generator basically start with the nodejs generated code and you would give it the type(xhr or websockets), transport, protocol, host, port, ssl and minify flags as options and I'd generate the whole thing as one file people can drop into their page and I'd only expose a client on the global namespace. Specifying the transport and protocol at the command line during generation means that you'd only bundle the transport and protocol you need in the final javascript file. The big change API wise for current users of the lib/js approach is that all callbacks will be node style callbacks where the first callback arg is a maybe error.

        Here are the eslint rules we use in lint-trap:
        https://github.com/uber/lint-trap/blob/master/rc/.eslintrc

        andrewdeandrade Andrew de Andrade added a comment - If we were to only use one, 100% go with eslint. It's way better than all the other linters out there. The reason for using both is that jscs has more whitespace rules than eslint. However, eslint is catching up on the whitespace rules that jscs has. The rules we used in lint-trap were designed isomorphic code in the npm ecosystem. i.e. any code that needs to work in both nodejs and in the browser via browserify. Some of the rules would not work with the code in lib/js. For example, the lib/js stuff relies on globals on the window object, which is something that we expressly forbid in rules we came up with. My personal opinion is that the code in lib/js and the generator for lib/js should be deprecated in favor of a single codebase in lib/nodejs. From that single codebase, we can use browserify to generate a "standalone" version of thrift (see the standalone documentation in the browserify readme https://github.com/substack/node-browserify ). The only additional work that would be required would be to take the generated nodejs client library and put it in a wrapper that exposes the same globals the client library currently exposes (since the nodejs library doesn't pollute the global namespace at all.) I can't think of a solid reason, except backwards compatibility (API) for maintaining both a lib/js and lib/nodejs. With a little work and reorganization, everything common between the browser and nodejs should be shareable. The browser.js file in THRIFT-2976 is my first attempt at a file that could be browserified with the standalone option. If I were to design the lib/js generator from scratch, I would have the generator basically start with the nodejs generated code and you would give it the type(xhr or websockets), transport, protocol, host, port, ssl and minify flags as options and I'd generate the whole thing as one file people can drop into their page and I'd only expose a client on the global namespace. Specifying the transport and protocol at the command line during generation means that you'd only bundle the transport and protocol you need in the final javascript file. The big change API wise for current users of the lib/js approach is that all callbacks will be node style callbacks where the first callback arg is a maybe error. Here are the eslint rules we use in lint-trap: https://github.com/uber/lint-trap/blob/master/rc/.eslintrc

        People

          Unassigned Unassigned
          hcorg Konrad Grochowski
          Votes:
          1 Vote for this issue
          Watchers:
          4 Start watching this issue

          Dates

            Created:
            Updated: