paho.mqtt.javascript icon indicating copy to clipboard operation
paho.mqtt.javascript copied to clipboard

modern JavaScript workflow

Open boneskull opened this issue 7 years ago • 9 comments

I spoke with @icraggs today about this project, and I'd like to help move it forward. We need to get MQTT v5 support going, but before that, I'd prefer to udpate the workflow: I'd like to see this fully integrated with npm and Node.js, using a typical toolchain to bundle for the browser. It's not a great developer experience for someone to have to install a Java toolchain to build a JS project.

Here's what needs to happen:

  • package.json
    • [ ] Add missing development dependencies (devDependencies); ensure no production dependencies are contained therein (and vice-versa)
    • [ ] Upgrade dependencies
    • [ ] Replace any dependencies that are not Node.js-compatible
    • [ ] Add missing fields: files, author, keywords, main, homepage, scripts (see below),version (see below), etc.
    • [ ] Modify description to reflect that it is, indeed, the actual Paho JS client
    • [ ] Leverage the browser field to point to bundle
    • [ ] Add repository field (Is it here or git.eclipse.org? Please advise)
    • [ ] Add license field. I expect that's EPL-1.0 as per the SPDX list
  • Linting
    • [ ] Lint JS and docstrings using the popular and flexible ESLint. Do not need Closure compiler. Configuration and plugins TBD, but semistandard is a reasonable place to start.
    • [ ] (Optional) Lint Markdown using Remark or similar tool
    • [ ] Add linting scripts to scripts prop of package.json (should also be invoked via test script)
  • Tests & CI
    • [ ] Execute tests via test prop of scripts
    • [ ] Update .travis.yml to build against supported versions of Node.js (I'm thinking v6.x or newer; v4.x will drop out of LTS soon)
    • [ ] Run tests in headless Chrome
    • [ ] Choose a target browser (e.g. IE11) in preparation for eventually running tests against it (on SauceLabs, for instance)
  • Build & bundling
    • [ ] Remove Maven and associated .xml files
    • [ ] Bundle for web with (probably?) Webpack
      • [ ] Prepend copyright banner
      • [ ] Minify via Uglify plugin
    • [ ] Add bundler to build script
  • Distribution
    • [ ] Add build script to prepublishOnly script; ensure distfiles are listed in files
    • [ ] Ensure bundle / dist files stay out of version control
    • [ ] Do not publish .zip archives within the tarball on npm :smile:
    • [ ] Use SemVer if not already
    • [ ] Create .zip file? Not sure how these are used. .zip archives are available on GitHub, and tarballs of published packages on npm. Please advise.
    • [ ] Add dist file to browser field as mentioned earlier
  • Documentation
    • [ ] Run JSDoc via e.g., doc npm script (we can publish this automatically on GitHub Pages at https://eclipse.github.io/paho.mqtt.javascript unless the API docs need to live where they currently live)
    • [ ] Add doc script to a complete build script
    • [ ] (Optional) Add README.md to the API docs
    • [ ] Update README.md; add any material changes to install process
    • [ ] Update CONTRIBUTING.md likewise (maybe move to .github/ along with CODE_OF_CONDUCT.md); it's already out-of-date
    • [ ] Figure out what to do with about.html
    • [ ] Note browser and Node.js compatibliity

Given I have entirely too much experience doing this stuff, it looks like a lot more work than it is. The two "hard" parts are likely going to be:

  • Running tests in headless Chrome, because I haven't set that up before. Hoping to avoid having to pull in Karma to do it
  • Adding Node.js support & bundling for browser could be painful

IMO it's best to knock this out all at once. I am completely new to this codebase, so there may be something I've missed. None of this should materially affect production usage of this package apart from potentially a different way to install it.

boneskull avatar Feb 16 '18 00:02 boneskull

This looks like a good plan @boneskull (although I am new to this library, I am not completely aware of its state). Would you be open to PRs?

kbariotis avatar Feb 16 '18 10:02 kbariotis

It appears jasmine needed an upgrade. Unfortunately, it's not backwards-compatible, so upgrading the tests from v1.x to v3.x syntax needs to happen. The jasmine-node module, which this project consumes, hasn't seen a new version in four (4) years.

boneskull avatar Feb 21 '18 04:02 boneskull

...also, the tests are written in such a way that a newer version is especially painful to use without pulling in async or promisifying stuff. otherwise it'll devolve into "callback hell", because some tests make multiple asynchronous assertions, which must happen in a certain order.

boneskull avatar Feb 21 '18 05:02 boneskull

Hi @boneskull This all sounds great, the Javascript client is definitely in need of some love and your plan looks pretty solid.

As you've already noticed the Jasmine tests are in a bit of a stuck state due to the out of date libraries. I was never able to find a nice way of converting them to using the newer version, do you think it would be better for us to bite the bullet and to re-write them from scratch using async?

Let me know if you need anything from me to accomplish this, and I'll process any PRs as soon as they come in.

jpwsutton avatar Feb 21 '18 10:02 jpwsutton

As you've already noticed the Jasmine tests are in a bit of a stuck state due to the out of date libraries. I was never able to find a nice way of converting them to using the newer version, do you think it would be better for us to bite the bullet and to re-write them from scratch using async?

IIRC async expects the first parameter to any given callback be an error, if present, so maybe that wouldn't work anyway, because the client's callbacks don't all conform to this style (see #141). So, that'd require breaking API changes--likely not the first thing on the TODO list.

Unless someone has written a codemod for this already, there's no recourse but to rewrite large swaths of the tests, I'm afraid. It's not easy, but we can:

  1. Try to eliminate cases where we are making multiple assertions per test, and split them up
  2. DRY; there is duplication we can zap
  3. Consider the value of each test, and eliminate tests which offer little, or rewrite others into unit tests

I expect a significant reduction in LoC w/o loss of coverage, which is a plus. Probably good to start with determining that coverage % as a baseline, and ensuring we don't drop below it.

boneskull avatar Feb 22 '18 04:02 boneskull

@kbariotis yes, we are always open to PRs. Also, I've been told that @boneskull may not be able to contribute to the project as he wished, so any contributions would be even more valuable.

icraggs avatar Feb 26 '18 14:02 icraggs

@icraggs @jpwsutton I am up for helping out with this, I have experience setting up Javascript Testing and Build tasks

tmulkern avatar Mar 15 '18 17:03 tmulkern

Is there any progress on the bundling for the web bullet?

mrwhy-orig avatar Apr 18 '18 16:04 mrwhy-orig

I started to structure the main file (class + import), used webpack, changed vars to let/conf, cleaned the eslint errors and adjusted some small things in the utility to being able to run it as chrome app (to possibly try tcp there soon). The libraries should be the same as in eclipse unide for piggyback CQs. Are you interested in a PR? https://github.com/eclipse/paho.mqtt.javascript/pull/158

ameinhardt avatar May 12 '18 21:05 ameinhardt