Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modernizing with ES6 classes, modules and smarter build process #103

Open
EliasHasle opened this issue Sep 29, 2019 · 3 comments
Open

Modernizing with ES6 classes, modules and smarter build process #103

EliasHasle opened this issue Sep 29, 2019 · 3 comments
Assignees

Comments

@EliasHasle
Copy link
Collaborator

EliasHasle commented Sep 29, 2019

Three.js are moving towards more modern source code, and I am certain it would improve both readability and maintainability to do so with vessel.js too. A bundler like Rollup.js can be configured to transpile to backwards-compatible code. Dependencies can be included by using NPM.

Classes:
Converting to classes is mostly straightforward, and will greatly improve readability and aid conceptual distinctions. (I notice, for example, that InsertCatenary is referred to as a function, yet the prototype is modified, which indicates it should be used as a constructor.)

Here is a conversion pattern. From:

function Ship(L,B,T) {
    this.L = L;
    this.B = B;
    this.T = T;
}
Ship.prototype = Object.create(Object.prototype);
Object.assign(Ship.prototype, {
    mockDisplacement: function() {
        return this.L*this.B*this.T;
    }
});

function VeryLongShip(B,T) {
    Ship.call(this, 1000, B, T);
}
VeryLongShip.prototype = Object.create(Ship.prototype);

to:

class Ship {
    constructor(L,B,T) {
        this.L = L;
        this.B = B;
        this.T = T;
    }
    mockDisplacement() {
        return this.L*this.B*this.T;
    }
}

class VeryLongShip(B,T) extends Ship {
    constructor(B,T) {
        super(1000, B, T);
    }
}

Modules:
Have each source file import only what it needs from other modules (typically a superclass and classes that must be constructed), and export only what other modules need (typically a single class). A single file, for instance named Vessel.js, serves as an entry point that provides all the global references. See the three.js project for an example on how it is done on a larger scale.

Bundling:
Three.js uses Rollup.js for bundling. I may aid you a bit on that one, since I have recent experience with using it, as Henrique/@hmgaspar hopefully knows.

Dependencies:
Dependencies can be included in a more principled way by using NPM. NPM can even install from GitHub, if the repos contains a valid package.json (with a compliant collection of files). package.json can configure which version is desired of each dependency. I can aid you with this too.

I have improved some of the scripts that are used in vesseljs and released them separately in Node/NPM package format:
https://github.com/EliasHasle/map-view-controls (a precursor is used in the GA example)
https://github.com/EliasHasle/patch-interpolation (minor changes)
You could also consider https://github.com/EliasHasle/kwargs.js

You will benefit from my later improvements if you use NPM to import dependencies. Node.js includes npm.

pinging @ferrari212 @DiogoKramel @icarofonseca

@hmgaspar
Copy link
Member

Hi Elias,

Good point, we just need to find the time to organise all this mess. Note that our package is already at NPM (https://www.npmjs.com/package/ntnu-vessel - vessel.js was taken!).

We can have a meeting in October to take a look on it and keep testing the examples.

@hmgaspar hmgaspar self-assigned this Sep 29, 2019
@hmgaspar
Copy link
Member

I'll call for a meeting to update us all on it.

@EliasHasle
Copy link
Collaborator Author

EliasHasle commented Sep 30, 2019

Note that classes are backward-compatible with the current prototype pattern. Some simple ones may be upgraded right away, independently of the others.

Note also that some conversions to classes will require a bit more effort than others. This is especially true in the cases where prototypes have method definitions within IIFE closures. We will need to move away from that pattern, like the three.js devs have just done. No big deal, actually, except maybe with the Hull class. (Manageable too.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants