opentype.js icon indicating copy to clipboard operation
opentype.js copied to clipboard

Uplift to ES6 classes and rollup bundling?

Open Pomax opened this issue 8 years ago • 19 comments

The codebase right now is actually extremely well organized and would be relatively easily uplifted to using ES6 classes, which would make every class a potential module that can easily be used by other open source projcets, or customized for opentype experiments like trying out new tables as part of a PoC prior to official proposal =)

Pomax avatar Jan 06 '17 18:01 Pomax

Aren't ES6 classes the new "bad part" of JS ?

fpirsch avatar Jan 08 '17 18:01 fpirsch

Subjectively: to some. Objectively: no.

As with everything in JS, you need to know how it works before you use it, or you're going to be using them wrong. This is often true in JS and classes are no different: ES6 classes are not the same as "class based object oriented programming" in things like C# or Java. It's still just prototypes. That's not "good" or "bad", that's just "welcome to JavaScript" (yet again - there are so many things in JS that you're going to do wrong if you think it does things like other languages you're familiar with). Would a different keyword have solved that? ffs would it ever have; but class was already a reserved keyword and TC39 picked that so sucks to be us, that's the keyword we get to use.

In this case: the class syntax is an incredibly powerful way to implement prototype inheritance without the pages and pages of code to do that well. It offers single word syntax where before we needed 10 lines of codes, and abstracts away all the "safety code" we need for raw prototype inheritance to ensure the right prototype's properties are accessed. So as long as you write your codebase fully aware of that, there is literally nothing bad about ES6 classes: they make the code infinitely more legible, and in this codebase specifically, infinitely easier to write things like plugins that introduce additional table parsing by making it trivial to write your own X implements SFNTTable or X implements CommonTable.

It gets bad when the programmers don't understand what they're using. If you use ES6 classes in the assumption you're now using classical inheritance, you done messed up, and the bad part is you.

Pomax avatar Jan 08 '17 18:01 Pomax

Isn't it precisely the point: keywords like class and implements make things look like java or C++ classes, and everybody wants to use them because it feels like good old java OOP, but nobody really understands what it is exactly. Btw, :+1: for plugins.

fpirsch avatar Jan 08 '17 21:01 fpirsch

@fpirsch If you already use prototype, class is a great way to organize this code in simple way. You can claim that you can't create truly multiple inheritance, but on the same time many will claim that inheritance is bad and multiple inheritance is multiple bad. Another thing, many IDEs do really well autocomplete only from using the class.

oriSomething avatar Jan 08 '17 21:01 oriSomething

Personally, I like ES6 classes. As @oriSomething mentioned, we're using prototype-based inheritance already anyway, so it's just renaming some things and cleanup.

What I don't like is Babel, but yeah, necessary evils and all that....

My favorite feature of ES6 is to use let and const everywhere instead of var. So, once we're using Babel I would like to go through the code and do var replacement everywhere.

fdb avatar Jan 09 '17 13:01 fdb

@fpirsch we can't control what TC39 picked as keyword, so it shouldn't really have been class but that ship has sailed: that's what TC39 decided on, so that's what it is, and that's what we now use (as for implements, that does not exist in JavaScript. There is an extends, and that is the correct keyword here, as one prototype class can most certainly extend another). Crucially: that also means that's what everyone now needs to learn: just because the word is the same in two languages, doesn't make it mean the same, just like in normal language.

So, does that invite mistakes? Of course, but really only if you don't bother to first learn how to use a language feature before you start implementing with it, much like a lot of other JS features. Lots of examples, but the most obvious one is that a ton of people also mistakenly use JavaScript arrays thinking that they can use them as associative arrays (instead of using plain objects) because that's what the languages they came from (PHP, Python) allow, and it "seems to work fine". Right up to the moment you try to iterate over all enumerable properties, or try to store using keys that collide with the array prototype functions =)

Does that make JavaSript arrays bad? Not really, in both cases the problem is the programmer not taking the time to make sure they understand the language they're using. JS classes are still a new feature, so not a lot of people (relatively) have actually looked up how they work, and a lot of people just go "I know what this is" because they saw it in a different language, then happily go on their way implementing things completely wrong based on the notion that what applies in one language also applies in another. That's not a problem with whatever syntax or methodology the language uses, that's programmers cutting corners and rightly getting confronted with the errors of their ways =)

@fdb I like babel a lot more now that it can be a quick step somewhere in the build process. With some very simple node scripts (especially since no react/jsx/etc stuff needs to be transpiled) three separate tasks for rollup, babel, and an aggregated task for "build":"npm-run-all rollup babel minify" makes things pretty nice.

Pomax avatar Jan 09 '17 17:01 Pomax

@fdb classes works on Node ≥ 4.x and every major browser exclude IE. Personally, I see very little problem with it, since there is already usage of bundler because the project is written with common JS. For Node, there is no babel need.

oriSomething avatar Jan 09 '17 17:01 oriSomething

Looking at http://caniuse.com/#search=classes IE would be the main stickling point really since IE11 has not yet been made obsolete by MS. Everything else should work fine (that said, having a legacy version of a library in pure ES5 is nice for things like older versions of Android phones, windows users who through no fault of their own are still forced to work on computers with old IE, etc)

Pomax avatar Jan 09 '17 17:01 Pomax

Hi 😉 I like the idea but I need an ES5 js file in the end for compatibility purposes... So 100% for ES6 if you could add that 🙏

Jolg42 avatar Jan 10 '17 09:01 Jolg42

I think we can make two versions if we need to: a version that is just rollup'd, in native ES6, and an ES5 file compiled with Babel. Oh and minfied versions of those two as well.

fdb avatar Jan 10 '17 09:01 fdb

I'm doing this now as a series of PRs:

  • First step is to move to ES6 modules #290.
  • Move from var to let / const.
  • Next step is to replace all MyClass.prototype.fn to ES6 classes.
  • I want to make sure that I have a clean bill of health from ESLint (with the Airbnb preset).

fdb avatar May 12 '17 10:05 fdb

As a nit: instead of going "from var to let/const", just go to "var and let and const", picking the correct keyword in the correct setting so as not to incur overhead that is irrelevant for the scoping and access required. var for function scoped variables, let for block scoped variables, and const for when you need a locked alias.

Ignoring the fact that var has a damn good reason to still be used (being the lowest-overhead alias allocator) is the weirdest decision people make when they uplift from ES5 to ES6 =)

Pomax avatar May 12 '17 18:05 Pomax

@Pomax Why is var still relevant?

  • Things related to performance stopped being relevant in latest versions
  • Using it as reassign global is an anti pattern that better avoided on es-module based version
  • Using it for hoisting is easily can lead to bugs

So, why actually using it? I admit I don't understand what you mean by saying being the lowest-overhead alias allocator. If you could please explain it?

oriSomething avatar May 13 '17 13:05 oriSomething

Short answer: because it's part of the language. They're not suddenly irrelevant because people have decided to ignore which each of the three allocators do, in the same sense that the fact that templating strings exist doesn't mean every string should now be a templating string, including strings that don't use templating.

So, as quick JS allocation explanation, var, let, and consts do similar, but different, things:

  • var: function scoped. Declared anywhere in a function, this variable persists until the end of that function. May be reassigned, may be redeclared, may be used redeclare. Redeclaring a variable with reassignment leaves that variable intact.
  • let: block scoped. Declared in a function, this variable persists until the end of that function. Declared inside a block inside a function, this variable persists until the end of that block only. May be reassigned, may not be redeclared, may not be used to redeclared.
  • const: function scoped. Declared anywhere in a function, this variable persists until the end of that function. May not be reassigned, may not be redeclared, may not be used to redeclare.
  • secret allocator number 4: not using an allocator. This invokes the var allocation mechanism, with global scope.

For all three: if declared in the global execution context, all three simply persist.

So to address your questions:

1: performance never stopped being relevant in codebases with a large amount of hot code, such as found in interpreters for densely packed binaries, a category OpenType parsers squarely fall into. The following code, for instance, wastes an incredible amount of time on allocation and garbage collection:

cmap4.segments.forEach(segment => {
  let start = segment.start;
  let end = segment.end;
  // compute delta or something similar
});

Advocating using let here, rather than vars declared and maintained outside of the loop is intentionally wasting a proportionally huge amount on time that can be spent on actually doing work rather than overhead:

function computeDeltasOrSomethingSimilar(segments) {
  var s=0, e=segments.length, segment, start, end;
  while(s++<e) {
    segment = segments[s];
    start = segment.start;
    end = segment.end;
    // compute delta or something similar
  }
}

In this case, the fact that there is a "nicer" allocator available can actually promote worse code by glossing over the fact that overhead still matters. In some codebases, that overhead is irrelevant given how much "regular" code kicks in, but in a binary parser like this, all those bits of unnecessary overhead add up fast over thousands of cascaded offset lookups.

2: this has nothing to do with using var, and everything to do with properly scoping your code. Modules in which execution context is automatically rescoped are an obvious safeguard, but are not more than a safeguard.

3: hmm, you may be thinking of something else: "hoisting" does not apply to variables, only to functions. You might be thinking of shadowing? E.g. function test() { ...; x = 5; ...; var x = 'moo'; ...;}. No hoisting occurs here, we're simply seeing two var allocations. The first creates an implicit var with global scope, the second then shadows that global variable with an explicit var that is function scoped from that point on.

Real hoisting kick in for functions, called hoisting because they get "hoisted up" from wherever you wrote them to the top of the execution context, before execution actually occurs. No matter how far down you wrote them, the interpreter will always first perform named function binding before actually running your code:

(function test() {
  console.log(test2());

  function test2() {
    return "functions are hoisted before function execution, so you see this text printed.";
  }
})()

I'll be happy to discuss this further, but then not in this issue, to prevent derailing it more than I've already caused it to be. You can find my email address in my github profile.

Pomax avatar May 14 '17 00:05 Pomax

The rollup bundling thing is now done!

fdb avatar May 15 '17 14:05 fdb

Thanks @Pomax for the explanation on vars. The main advantage for me in using let and const is to check if I've scoped variables correctly.

I agree on the performance considerations for this library. However, as I understand it, nothing blocks you from moving up a let outside of a looping block and at the top of the function, which would eliminate the overhead.

In the cmap example, I think the main overhead would be the anonymous function, not necessarily the scoping. I've tested this in jsperf and it seems the overhead of the tightly scoped let is negligible: https://jsperf.com/let-scoping-performance/1

capture

Please let me know if I'm missing something 😉

fdb avatar May 15 '17 14:05 fdb

@fdb nice: real numbers always trump "what the spec suggests should happen"! Another good thing to test is the perf on Node.js (you'd think it'd be the same as Chrome, but it sometimes optimizes differently)

Pomax avatar May 15 '17 18:05 Pomax

I don't see why the spec would say anything different. 'let' variable holders are not in heap and thus garbage collected, but are on the stack. And stacks are fast, modern processors are very stack optimized and a good preprocessor bundling the code on release can uplift these variables anyway.

axkibe avatar May 16 '17 07:05 axkibe

@fdb @Jolg42 I'd like to help with this. I'm thinking of doing the prototype -> es6 classes conversion. Will I need to make any changes in the pipeline? or will making changes in src be enough?

LabhanshAgrawal avatar Jun 22 '20 07:06 LabhanshAgrawal