Add ES6 module support #214

keego · 2018-02-28T23:12:37Z

This adds support for ES6 modules (fixing #197) which are commonly used in modern javascript development practices. This uses the pkg.module route suggested by the guys over at Rollup which seems to be the most standard way to add ES6 module support without making breaking changes.

The main changes are an amendment to package.json:

   "main": "./src/Ros3D.js",
+  "module": "./src/Ros3D.esm.js",

And duplicating Ros3D.js with an appended export default ROS3D at the bottom.

I recognize this isn't very DRY so I'm open to alternative methods of re-exporting Ros3D.js but I figure this at least gets the ball rolling.

UPDATE: Changes required for this PR ended up growing quite a bit, checkout my comment below for details.

Notes on importing:

Importing as a module

import * as ROS3D from 'ros3d'
// Depending on your module bundler, this may either target:
// - ros3d.cjs.js - an ES5 compliant CommonJS module
// - ros3d.esm.js - an ES5 compliant ES module

Importing into an html document (should be the same):

<!-- unminified - easier build for debugging -->
<script src="path/to/ros3d/build/ros3d.js"></script>
<!-- minified - smaller build for production -->
<script src="path/to/ros3d/build/ros3d.min.js"></script>

Some additional examples have been added to show different import semantics as well.

Closes #197

keego · 2018-02-28T23:40:14Z

Ah this is going to be less trivial than I thought. Seems like your guys' Grunt pipeline will need some adjustments too...

keego · 2018-03-01T03:10:39Z

Okay so after doing some digging, I found some interesting results.

The general structure this project uses of implicitly depending on globals may be conducive to the early web days of global namespace clobbering, but makes it kind of sketchy to support ES6 modules without significant code changes and/or breaking backwards compatibility. Plus I'm curious how it affects performance as I'm not sure if the ES6 modules will be able to tree shake much.

That said, it turns out that its fairly simple to extend the current build process to have an additional build target that is ES6 compliant. Simply concatenating some imports, build/ros3d.js, and an 'export default ROS3D' actually gets us pretty far in terms of turning global dependencies into modular ones.

So while this isn't the prettiest solution, it shouldn't have any effect on current users or the current development effort. Changes required to keep supporting the ES6 module should be minimal, likely requiring little to no changes to ES6 wrapper / grunt config.

keego · 2018-03-01T03:29:50Z

PLEASE DONT MERGE THIS YET BTW

I'm referencing these changes in my current project but I want to make sure to work out any and all kinks beforehand

keego · 2018-03-03T01:33:15Z

Yay more progress!

Choosing a module bundler

Looking at Webpack vs Rollup, I think Rollup may be a better fit for bundling this library due to its simplicity and output bundle size.

(See this excellent SO answer and this medium post for more info)

TLDR on the articles / my reasoning:
While Webpack has a lot more features, they're generally more helpful for web apps than libraries (HMR, lazy-loading modules, etc.). However, code-splitting (aka tree-shaking) would be nice to have for ros3d consumers, but I believe they should get that anyway since we'd be exporting an ES6 module and ES6 modules natively support code-splitting.

What needs to change

Rollup has an awesome online REPL which makes it easy to test things out. You can select the desired module format for the output bundle as well. "es" is shows the ES6 bundle output (which would be imported by web apps via npm) and "iife" demonstrates a browser compatible bundle for including via a script tag.

This example demonstrates the current module definition semantics and how they make bundling difficult (global depencies aside).

This example shows how modern JavaScript libraries are written and bundled. I believe this is how we should be doing things.

@sevenbitbyte @jihoonl Thoughts on moving forward with the above mentioned format?

Sidenote: Rollup doesn't natively transpile ES6 to ES5, a necessary step for supporting older browsers. However, rollup-plugin-babel solves this problem for us :)

AustinDeric · 2018-03-03T15:57:00Z

@keego, this is great work thanks! I tried to test this implementation, as i am trying to do something similar. I am building a react app with webpack.

My steps are to install this repo

npm install https://github.com/keego/ros3djs.git#develop

Then, i import this ros3d node module into the react app (which is very similar to the examples, for the purpose of testing this code):
https://gist.github.com/AustinDeric/1a4c2b9bb6bae68eab230c33d276d7d5

the console log of the ROSLIB.Ros object looks great and connects to my ros bridge.

However the ROS3D.Viewer object produces the following error:

TypeError: _ros3d2.default.Viewer is not a constructor

UPDATE: I see that @keego's comment above cuts to the problem. webpack is the way to go. It looks like ros3djs will need to be heavily modified to accomplish this and would probably need a hard fork. It also worth discussing how this will work with rclnodejs.

A good guide (and reasons to migrate) for the needed modifications: migrate-to-webpack-from-grunt-bower-legacy-build-system

keego · 2018-03-05T01:12:15Z

@AustinDeric My forked branch is still broken and will be for a while. I wanted to do some investigating before getting it ES6 compatible, especially in light of seeing just how much modification is necessary to make the change. I'll be posting here when I get it working.

How is ros3djs currently used with rclnodejs? Do you have some example code? I don't have much to test my changes against.

Also, I'd like to clarify I'll be moving forward with Rollup not Webpack.

jihoonl · 2018-03-05T01:18:31Z

Too sad that I don't have enough time for proper review at the moment.. @sevenbitbyte or @viktorku could you guys take a look?

BTW, I would love to add ES6 module support. Thanks for the PR!

sevenbitbyte · 2018-03-05T07:26:52Z

Gruntfile.js

@@ -6,6 +6,10 @@ module.exports = function(grunt) {
      build: {
        src  : ['./src/*.js', './src/**/*.js'],
        dest : './build/ros3d.js'
+      },
+      build_esm: {
+        src  : [ './src-esm/ESMHeader.js', './build/ros3d.js', './src-esm/ESMFooter.js'],


Not required but might be more concise to keep all code in ./src directory rather than creating a new one. Do you need a new directory to avoid a conflict with the existing grunt job for instance?

Nope, src-esm will be removed. I was toying with the idea of wrapping the original src instead of rewriting it. I imagine we should just move to writing ES6 only now?

sevenbitbyte · 2018-03-05T07:27:28Z

Gruntfile.js

@@ -32,7 +37,7 @@ module.exports = function(grunt) {
      build: {
        src: './build/ros3d.js',
        dest: './build/ros3d.min.js'
-      }
+      },


Watch out for dangling comma... or maybe linter is fine with this

Current jshint rules are unopinionated about dangling commas. After looking around I haven't seen dangling commas elsewhere so I'll stick to the defacto no-trailing-comma style for this codebase.

keego · 2018-03-06T00:46:26Z

Sidenote: @jihoonl Where is ROS3D.COLLADA_LOADER_2 defined? I've found various references to it but can't seem to find it defined anywhere.

jihoonl · 2018-03-06T03:00:14Z

COLLADA_LOADER_2 is not available anymore. It is replaced by https://github.com/RobotWebTools/ros3djs/blob/develop/examples/js/ColladaLoader.js while three.js code base update in #202

keego · 2018-03-07T22:56:08Z

@jihoonl May I go ahead and remove remaining references to ROS3D.COLLADA_LOADER_2 (such as in InteractiveMarker) then?
i.e.
from:

var loader = options.loader || ROS3D.COLLADA_LOADER_2;

to:

var loader = options.loader;

jihoonl · 2018-03-07T22:56:56Z

go ahead.

keego · 2018-03-07T23:08:01Z

Sidenote, I'll be at SCaLE until Tuesday, so my focus will be elsewhere until then.

…g. 1 - removed explicit super.super() call from InteractiveMarker. 2 - Made Particles explicitly derive from THREE.Object3D.

…f codebase

…erived classes

…ed a relevant comment

…module extensions

…l imports in browsers

… for es6 transpiled output

jihoonl · 2018-04-08T04:16:14Z

@keego hello. Any updates on this?

keego · 2018-05-01T02:30:55Z

Glad to help @jihoonl. I made some changes to the build process to get it working with the current CI pipeline (which also lead to GitHub auto closing this issue, oops). It looks like you guys are building with a very old version of Node, which isn't liking the transpiler.

Any chance you could update the Travis CI config to build with the latest stable Node version (v8.9.4)?

AustinDeric · 2018-05-01T15:06:03Z

I ended up not needing the import ROSLIB statement, i added webpack to keego's work and build the .cjs file and that works really nice. The I will make a PR with this work once this PR get merged.
Broken URDF, i am still looking into this, i will try again. I currently have no global includes in the html file (yay!). It could very well be specific to my setup. If not i will make a dockerfile and package to share with the team.
Which file is intended to be the compiled output to ES5? It looks like it would be ./build/ros3d.cjs.js but that file is not ES5 compliant and cannot be used as an external dependency (hence my webpack fix).

keego · 2018-05-02T03:47:48Z

@AustinDeric ros3d.cjs.js wasn't es5 compliant. Turns out it still used some es6 features despite being in commonjs format. I pushed some changes that make sure the commonjs module is in es5.

AustinDeric · 2018-05-02T16:47:08Z

@keego is a hero! new changes to develop branch work great! URDF problem fixed too:

the only change i had to make was in package.json "module": "./build/ros3d.csj.js", or else the import looks for the esm file which i can't minify because its not es5 compatible.

For reproducibility:

npm install git+https://github.com/keego/ros3djs.git#develop (commit 40c3cbc)
change package.json module line to "module": "./build/ros3d.csj.js"
previous gist works perfectly (https://gist.github.com/AustinDeric/f6b7ccaf24cffedea7d6c5a5eb61aa2b)

keego · 2018-05-02T19:05:43Z

@AustinDeric awesome! Thanks for testing things out!

As for the package.json changes, I was using the main and module fields as recommended from Rollup's guide on pkg.module, but forgot that while module is supposed to target javascript that uses ES6 module syntax (i.e. import/export), the rest of the module has to use ES5 language features.

Just pushed changes that fix this.

keego · 2018-05-18T01:18:21Z

@jihoonl Any chance you could update the Travis CI config to build with the latest stable Node version (v8.9.4)? The (much) older version of Node being used doesn't like the transpiler.

keego · 2018-05-24T23:31:10Z

@jihoonl @sevenbitbyte Just realized that the Travis CI config is literally in the repo :face_palm: so I just specified travis ci to use 8.9.4 instead of 0.10. All lights green 👍

Also, could one of you make an develop-es6 branch on this repo so I can PR my ES6_ONLY branch into it? That way people can start targetting it for new PRs and, more importantly, stop targetting the non-es6 branch. Should be a good way to prevent clobbering existing work being done.

jihoonl · 2018-07-03T04:28:01Z

aight

jihoonl · 2018-07-03T04:29:33Z

thanks for long waiting.. though I couldn't test it properly, i decided to merge it to move forward.

jihoonl · 2018-07-03T04:30:31Z

Also,. I have created develop-es6 branch as you requested.

keego · 2018-07-03T23:30:40Z

Oh great, thanks! Yeah no worries, I figured you were busy.

keego · 2018-07-04T00:11:19Z

@jihoonl I documented the transition process (#228) and started the PR for updating develop-es6 (#229). Should be good to go at your own pace 👍 feel free to mention or email me for questions / comments / help.

Cheers 🍻

keego added 2 commits March 4, 2018 21:58

Added Rollup

8f35c9e

Added yarn lockfile

e517837

sevenbitbyte reviewed Mar 5, 2018

View reviewed changes

Removed references to COLLADA_LOADER_2

55491d5

keego added 13 commits April 6, 2018 20:29

Added es6 transpiler and rollup config, working on transpiling

60fdb62

Touched up a couple potential mistypes prevent transpiler from workin…

ec26ae0

…g. 1 - removed explicit super.super() call from InteractiveMarker. 2 - Made Particles explicitly derive from THREE.Object3D.

Rewrote Particles' method signatures in format consistent with rest o…

e98dc5a

…f codebase

Refactored updateMatrixWorld to be more statically analyzable

7f8372d

ES6 modules properly compiling, Working on runtime errors

204239f

Moved 'that' assignments happen *after* calls super constructors in d…

bf13a07

…erived classes

Added missing super constructor call

3ad1d51

Removed assignment to read-only property causing a runtime error, add…

c7cebac

…ed a relevant comment

Moved all super constructor calls to preceed any use of 'this'

533b1d4

Added shims for THREE and THREE extensions to support es6 compatible …

4d1bdd9

…module extensions

Cleanup and added examples for single page applications and using htm…

3daa122

…l imports in browsers

Moved es6 support files to es6-support folder and renamed destination…

12bc8e8

… for es6 transpiled output

Moved shims to top level

7dc4adf

keego force-pushed the develop branch from 05dbe3d to cdf9966 Compare April 30, 2018 22:26

keego reopened this Apr 30, 2018

keego force-pushed the develop branch 3 times, most recently from b500a93 to d619775 Compare April 30, 2018 22:53

Updated outdated grunt plugins

5768b2a

keego force-pushed the develop branch from d619775 to 5768b2a Compare April 30, 2018 23:03

keego added 2 commits April 30, 2018 19:04

Fixed linter errors

bff42f6

Updated grunt build process to build es6 output

346ad62

keego added 2 commits May 1, 2018 20:11

Switched from jshint to eslint, migrating rules and fixing lint issues

563430c

Updated commonjs target to be es5

40c3cbc

Fixed pkg.module to use es6 module syntax, but es5 language features

e938fe9

keego added 3 commits May 4, 2018 16:08

Fixed bug in Points class

d38c332

Switched from const to var for es5 compatibility

b426742

Added PointCloud2 example for angular app

cd53ca7

Updated node version use by CI server

a4453e7

jihoonl merged commit a816d67 into RobotWebTools:develop Jul 3, 2018

keego mentioned this pull request Jul 3, 2018

Move to using ES6 source code #228

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ES6 module support #214

Add ES6 module support #214

keego commented Feb 28, 2018 •

edited

Loading

keego commented Feb 28, 2018

keego commented Mar 1, 2018

keego commented Mar 1, 2018 •

edited

Loading

keego commented Mar 3, 2018

AustinDeric commented Mar 3, 2018 •

edited

Loading

keego commented Mar 5, 2018

jihoonl commented Mar 5, 2018

sevenbitbyte Mar 5, 2018

keego Mar 5, 2018

sevenbitbyte Mar 5, 2018

keego Mar 5, 2018

keego commented Mar 6, 2018 •

edited

Loading

jihoonl commented Mar 6, 2018

keego commented Mar 7, 2018

jihoonl commented Mar 7, 2018

keego commented Mar 7, 2018 •

edited

Loading

jihoonl commented Apr 8, 2018

keego commented May 1, 2018

AustinDeric commented May 1, 2018 •

edited

Loading

keego commented May 2, 2018

AustinDeric commented May 2, 2018 •

edited

Loading

keego commented May 2, 2018

keego commented May 18, 2018

keego commented May 24, 2018 •

edited

Loading

jihoonl commented Jul 3, 2018

jihoonl commented Jul 3, 2018

jihoonl commented Jul 3, 2018

keego commented Jul 3, 2018

keego commented Jul 4, 2018

Add ES6 module support #214

Add ES6 module support #214

Conversation

keego commented Feb 28, 2018 • edited Loading

keego commented Feb 28, 2018

keego commented Mar 1, 2018

keego commented Mar 1, 2018 • edited Loading

keego commented Mar 3, 2018

Choosing a module bundler

What needs to change

AustinDeric commented Mar 3, 2018 • edited Loading

keego commented Mar 5, 2018

jihoonl commented Mar 5, 2018

sevenbitbyte Mar 5, 2018

Choose a reason for hiding this comment

keego Mar 5, 2018

Choose a reason for hiding this comment

sevenbitbyte Mar 5, 2018

Choose a reason for hiding this comment

keego Mar 5, 2018

Choose a reason for hiding this comment

keego commented Mar 6, 2018 • edited Loading

jihoonl commented Mar 6, 2018

keego commented Mar 7, 2018

jihoonl commented Mar 7, 2018

keego commented Mar 7, 2018 • edited Loading

jihoonl commented Apr 8, 2018

keego commented May 1, 2018

AustinDeric commented May 1, 2018 • edited Loading

keego commented May 2, 2018

AustinDeric commented May 2, 2018 • edited Loading

keego commented May 2, 2018

keego commented May 18, 2018

keego commented May 24, 2018 • edited Loading

jihoonl commented Jul 3, 2018

jihoonl commented Jul 3, 2018

jihoonl commented Jul 3, 2018

keego commented Jul 3, 2018

keego commented Jul 4, 2018

keego commented Feb 28, 2018 •

edited

Loading

keego commented Mar 1, 2018 •

edited

Loading

AustinDeric commented Mar 3, 2018 •

edited

Loading

keego commented Mar 6, 2018 •

edited

Loading

keego commented Mar 7, 2018 •

edited

Loading

AustinDeric commented May 1, 2018 •

edited

Loading

AustinDeric commented May 2, 2018 •

edited

Loading

keego commented May 24, 2018 •

edited

Loading