Move to ES6, Rollup bundler for ESM/CJS bundles #128

ShukantPal · 2020-08-02T15:13:08Z

Changes

Move to ES6 classes, let/const
ESLint added & auto-corrected over the codebase
Move to rollup bundler to generate CommonJS, ESM bundles as well

ShukantPal · 2020-08-02T15:13:33Z

ShukantPal · 2020-08-02T17:58:36Z

@Pomax I wanted to know if you were okay with some of the changes, specifically ESLint & Rollup?

Pomax · 2020-08-06T21:42:05Z

There should not be any need for babel, if we're redoing this as proper ES6 code: adding a type: "module" to the package.json should be enough. And if folks need their codebase converted to ES5 during a build step, then their own config should be able to take care of that just fine.

Pomax · 2020-08-06T21:42:45Z

README.md

@@ -11,11 +11,11 @@ or read the souce (`./lib` for the library code, start at `index.js`).

 ## Use

-`var Bezier = require('bezier-js')` and off you go.
+`const { Bezier } = require('bezier-js')` and off you go.


this should definitely be import { Bezier } from "bezier-js"; now =)

Pomax · 2020-08-06T21:43:22Z

README.md


 ## Use in the browser

-Load the toplevel `bezier.js` the same way you would any other browser script, using a `<script src="..."></script>` declaration.
+Load `dist/bezier.js` the same way you would any other browser script, using a `<script src="..."></script>` declaration.


and this should probably no longer be necessary, if we're using proper ES6 (both the browser and node supposed es modules)

Pomax · 2020-08-06T21:44:50Z

package.json

    "browserify": "^9.0.3",
    "chai": "^3.2.0",
    "chai-stats": "^0.3.0",
+    "eslint": "^7.6.0",


Let's use prettier rather than eslint. I'd rather have an opinionated but zero-conf linter than a free form linter that requires its own .rc file to do something sensible.

Pomax · 2020-08-06T21:46:05Z

package.json

-    "webpack": "^1.12.13"
+    "rollup": "^2.23.0",
+    "rollup-plugin-sourcemaps": "^0.6.2",
+    "rollup-plugin-terser": "^6.1.0"


no need to preemptively minify: if folks need minification, that would be a step their own config takes care of. Preemptive minification can in fact lead to worse results.

Pomax · 2020-08-06T21:47:57Z

package.json

-    "uglify-js": "^2.4.17",
-    "webpack": "^1.12.13"
+    "rollup": "^2.23.0",
+    "rollup-plugin-sourcemaps": "^0.6.2",


I honestly don't see much value in adding this: you don't minify code in your dev environment, so you don't need sourcemaps there, and prod should never load insanely huge debug-only files by default, so while an interesting idea, let's not add source mapping.

Pomax · 2020-08-06T22:00:02Z

src/normalise-svg.js

@@ -3,33 +3,33 @@
 * and full commands, rather than relative coordinates
 * and/or shortcut commands.
 */
-function normalizePath(d) {
+export function normalisePath (d) {


This is a singleton file, exposing a single function: this can be a default export.

Suggested change

export function normalisePath (d) {

export default function normalisePath (d) {

Pomax · 2020-08-06T22:00:32Z

src/svg-to-beziers.js

@@ -1,33 +1,33 @@
-var normalise = require("./normalise-svg.js");
+import { normalisePath as normalise } from './normalise-svg';


with the export made a default, this can just "stay" what it was:

Suggested change

import { normalisePath as normalise } from './normalise-svg';

import normalise from './normalise-svg';

Pomax · 2020-08-06T22:01:35Z

src/svg-to-beziers.js

+  var term;
+  var matcher = new RegExp('[MLCQZ]', '');
+  var segment;
+  var values;
+  var segments = [];
+  var ARGS = { C: 6, Q: 4, L: 2, M: 2 };


ES6 should not be using var? ;)

Pomax · 2020-08-06T22:02:23Z

src/utils.js

+import { Bezier } from './Bezier';
+
+// math-inlining.
+const abs = Math.abs;


Again, ES6 is better than ES5 here.

const { abs, cos, sin, etc, etc } = Math;

Pomax · 2020-08-06T22:03:06Z

test/test.js

-var utils = Bezier.getUtils();
-var assert = require("chai").use(require("chai-stats")).assert;
-
+const Bezier = require('../lib/bezier');


this should definitely also become normal ES6 module code.

Pomax · 2020-08-06T22:12:53Z

I think we need to restart this but on a different footing. If bezier.js gets uplifted to ES6, then that should be fully committed to: no more require, nothing that generates ES5, and ES5 code patterns that have been replaced with better ones in ES6, replaced with those better ones. Also, for linting, let's use https://prettier.io, without any customization: whatever it considers good code is simply what the code will look like (and that can change from version to version, but that's fine).

Let's restart this but do this in steps:

let's start a new es6 branch, with just the /lib and /test code, and uplift those files first, with an npm script for linting
1. initial super-minimal rewrites just so that things use import/export without any other code changes
2. then, switch things over to normal modern class syntax, without any code changes inside functions
3. then, start rewriting function internals to ES6 syntax and patterns.
switch the testing to jest, because it's still the weird ancient hand-written testing atm.
once that work's done, let's add in rollup so that a browser "bundle" can be created. Note that minifcation is not necessary here: any sane server is going to serve that file with gzip, which is superior to any kind of JS based minification solution out there, and can actually perform worse on pre-minified code.

ShukantPal · 2020-08-06T22:55:13Z

I'll redo this then I guess ;)

Pomax · 2020-08-07T05:08:35Z

Just remember to not "do everything": do step 1, file a PR, let's get that merged into https://github.com/Pomax/bezierjs/tree/es6 (I created a new branch for us to work against). Then we can iteratively improve that without ending up with one giant PR that is impossible to easily review =)

"Write small changes, file a PR, land the PR, write more small changes" is infinitely better than "write large changes, discover something fundamental invalidates all the work" ^_^;;

Move to ES6, Rollup bundler for ESM/CJS bundles

e3c50fb

Pomax requested changes Aug 6, 2020

View reviewed changes

ShukantPal closed this Aug 6, 2020

Pomax mentioned this pull request Aug 7, 2020

Move to ES6 #127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to ES6, Rollup bundler for ESM/CJS bundles #128

Move to ES6, Rollup bundler for ESM/CJS bundles #128

ShukantPal commented Aug 2, 2020

ShukantPal commented Aug 2, 2020

ShukantPal commented Aug 2, 2020

Pomax commented Aug 6, 2020

Pomax Aug 6, 2020

Pomax Aug 6, 2020

Pomax Aug 6, 2020

Pomax Aug 6, 2020

Pomax Aug 6, 2020

Pomax Aug 6, 2020

Pomax Aug 6, 2020

Pomax Aug 6, 2020

Pomax Aug 6, 2020

Pomax Aug 6, 2020

Pomax commented Aug 6, 2020 •

edited

Loading

ShukantPal commented Aug 6, 2020

Pomax commented Aug 7, 2020

	export function normalisePath (d) {
	export default function normalisePath (d) {

		@@ -1,33 +1,33 @@
		var normalise = require("./normalise-svg.js");
		import { normalisePath as normalise } from './normalise-svg';

	import { normalisePath as normalise } from './normalise-svg';
	import normalise from './normalise-svg';

Move to ES6, Rollup bundler for ESM/CJS bundles #128

Move to ES6, Rollup bundler for ESM/CJS bundles #128

Conversation

ShukantPal commented Aug 2, 2020

Changes

ShukantPal commented Aug 2, 2020

ShukantPal commented Aug 2, 2020

Pomax commented Aug 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pomax commented Aug 6, 2020 • edited Loading

ShukantPal commented Aug 6, 2020

Pomax commented Aug 7, 2020

Pomax commented Aug 6, 2020 •

edited

Loading