Fix scala.js deprecation warnings

[Mingun]

[generalmimon]

I need to check this once I'm on my computer, if it works as before or not.

[Mingun]

Hey! Let's finish this (and other PRs) before end of year! 😃 @GreyCat, @generalmimon, @dgelessus

[Mingun]

I've found that probably changing factory to compiler is wrong. According my analysis, current master does the following:

• root.KaitaiStructCompiler is a kaitai struct compiler constructor function, should be used as KaitaiStructCompiler() or new KaitaiStructCompiler() which gives us a compiler object
• module.exports the same as root.KaitaiStructCompiler
• define accepts a factory that produces kaitai struct compiler constructor function

After upgrading scala-js and do not touching factory stuff:

• root.KaitaiStructCompiler is a compiler object itself, should be used as KaitaiStructCompiler
• module.exports the same as root.KaitaiStructCompiler
• define accepts a factory that produces a compiler object

Such a change seems reasonable and I think we shouldn't does anything with that. As I see, nobody in kaitai_struct repos using this generated file, but I've not checking web-IDE

[generalmimon]

Such a change seems reasonable and I think we shouldn't does anything with that.

Well, I wholeheartedly agree, as the stuff like

const KaitaiStructCompiler = require('kaitai-struct-compiler');
const compiler = new KaitaiStructCompiler();
// compiler.compile(...);

that shows the common usage at the moment actually doesn't make any sense - KaitaiStructCompiler is just some 0-arg trivial function like function() { return MainJs; } so invoking it with new is just pure nonsense, and the whole step compiler = KaitaiStructCompiler() is completely redundant as well.

But I'm reluctant to make such a daring BC break, because kaitai-struct-compiler at npm is used quite a lot in the wild (as you can see from the download stats, it has ~300 downloads per week, whatever that means) and this change is guaranteed to break all code using it.

As I see, nobody in kaitai_struct repos using this generated file, but I've not checking web-IDE

Well, at the very least, GitHub shows so-called dependents by scanning the package.json files: https://github.com/kaitai-io/kaitai_struct_compiler/network/dependents

And yeah, the Web IDE is essentially based on the Scala.js bundle of KSC, I think that actually the Web IDE was the reason why the Scala.js target of the KSC was introduced in the first place :)

So we've got a dilemma here of simplifying the usage but breaking the BC and just leaving it as it is for now (in the not-so-ideal state). I mean - the fix is incredibly simple (you just remove the calling brackets ()), but you still have to know that you need to do it, and it can be really annoying to be an unsuspecting user, run npm update and wonder why the app just stopped working. But yeah, I know - we need to do the BC breaks at some point 🙂

I see that you added an info to the README, that's great, but I wonder whether we can also automatically show a warning in the console right after a user updates the kaitai-struct-compiler package to the version where the BC break happens. Would something like this work?

warn-about-constructor-bc-break.js

console.warn(
'kaitai-struct-compiler: WARNING - you\'ve just installed the 0.10.0-SNAPSHOT20201217+ version. \
If you\'re coming from an older version, make sure to update your code - \
the usage changed from "(new KaitaiStructCompiler()).compile(...)" to "KaitaiStructCompiler.compile(...)".\n'
);

package.json

{
  "version": "0.10.0-SNAPSHOT20201217-...",
  "scripts": {
    "postinstall": "node warn-about-constructor-bc-break.js"
  }
}

I think that this warning can be quite useful for the users.

[Mingun]

But I'm reluctant to make such a daring BC break, because kaitai-struct-compiler at npm is used quite a lot in the wild (as you can see from the download stats, it has ~300 downloads per week, whatever that means) and this change is guaranteed to break all code using it.

Well, kaitai-struct-compiler follows semver semantics, so that's not a big problem. Until version 1.0 breaking changes is allowed when changing minor version. In the long-term perspective is is better to fix such kind of things before we reaching version 1.0 because versions 0.x are just for this! Anyway, in production code you shouldn't depends on the unstable tags, so if you depends on something like latest version, I think, you are understands all pros & cons.

So we've got a dilemma here of simplifying the usage but breaking the BC and just leaving it as it is for now (in the not-so-ideal state). I mean - the fix is incredibly simple (you just remove the calling brackets ()), but you still have to know that you need to do it, and it can be really annoying to be an unsuspecting user, run npm update and wonder why the app just stopped working. But yeah, I know - we need to do the BC breaks at some point 🙂

I definitely sure that we should do that. In any case when you upgrade your dependencies to next minor/major version you shouldn't expect that all will work as before 🙂

I see that you added an info to the README, that's great, but I wonder whether we can also automatically show a warning in the console right after a user updates the kaitai-struct-compiler package to the version where the BC break happens. Would something like this work?

I've add this to the PR, but I think it will not be so useful, because it seems that this script will not run on npm update but these users is the main contingent that should be notified. I was unable to find instructions to display a message when npm update, not when npm install. It is possible, of course, that the update includes installation... For that reason I've add this warning as separate commit, so If after all you decide that it useless, just skip it when merge.

[Mingun]

Ping