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

Support header/footer variants for javascript targets in ksc #267

Open
fallenoak opened this issue Sep 26, 2017 · 9 comments
Open

Support header/footer variants for javascript targets in ksc #267

fallenoak opened this issue Sep 26, 2017 · 9 comments

Comments

@fallenoak
Copy link

fallenoak commented Sep 26, 2017

While working on tidying up kaitai-struct-loader, it's dawned on me that the UMD header/footer isn't necessary within webpack.

Ideally, the output for a ksy being compiled for use in webpack would look like vanilla CJS:

var SomeKsyImport = require('./SomeKsyImport');

function TestKsy(_io, _parent, _root) {
  this._io = _io;
  this._parent = _parent;
  this._root = _root || this;

  this.foo = new SomeKsyImport(this._io);
}

module.exports = TestKsy;

Perhaps we could signal this with a command line flag? --js-module-format cjs for example?

Another useful variant would be ES6 modules (which, ostensibly, webpack 2 can also consume):

import SomeKsyImport from './SomeKsyImport';

function TestKsy(_io, _parent, _root) {
  this._io = _io;
  this._parent = _parent;
  this._root = _root || this;

  this.foo = new SomeKsyImport(this._io);
}

export default TestKsy;

Maybe --js-module-format es6?

@fallenoak fallenoak changed the title Support customized headers/footers for javascript targets in ksc Support header/footer variants for javascript targets in ksc Sep 26, 2017
@GreyCat
Copy link
Member

GreyCat commented Sep 26, 2017

LGTM. We'd likely use --javascript-... prefix for CLI arguments for consistency.

Can you clarify:

  • What is "vanilla CJS" and how widespread is that name (i.e. I don't see to find anything relevant by googling that query)
  • How do we call current "format"? Would umd work?
  • Shall I resurrect "previous" format, and if yes, how shall we call it? I mean, this one:
var TestKsy = (function() {
  function TestKsy(_io, _parent, _root) {
    this._io = _io;
    this._parent = _parent;
    this._root = _root || this;

    this._read();
  }
  TestKsy.prototype._read = function() {
    // ...
  }

  return TestKsy;
})();

// Export for amd environments
if (typeof define === 'function' && define.amd) {
  define('TestKsy', [], function() {
    return TestKsy;
  });
}

// Export for CommonJS
if (typeof module === 'object' && module && module.exports) {
  module.exports = TestKsy;
}

@GreyCat
Copy link
Member

GreyCat commented Sep 26, 2017

As far as I understand, generally we could do the following formats:

  • umd — format that ksc generates now
  • commonjs — basically as described above by @fallenoak (did you mean that "CJS" = "CommonJS"?)
  • commonjs+amd — "previous" format, as described above
  • amd — as described here, or derived by stripping down commonjs+amd
  • none — no require headers at all, no exports; might be useful for manual glueing of stuff for web browser usage or something (?)
  • es6 — as described above; I wonder if we need any KaitaiStream imports there?

@koczkatamas
Copy link
Member

CJS is CommonJS.

@fallenoak could you confirm that webpack supports es6, so we only need umd and es6?

UMD works AFAIK in every runtime environment (browsers, node, etc). So amd, commonjs+amd should not be required in itself.

Problem comes when you try to preprocess these files, and not run them. But in that case I'd expect every modern tool (as preprocess tools are usually young tools) to support ES6 as this is the future.

We only need CJS (commonjs) support if a wide-spread preprocess tool does not support ES6 modules yet.

Of course if it's easier to implement support for all of them than arguing what to support, then ignore my comments and implement all options.

@GreyCat
Copy link
Member

GreyCat commented Sep 26, 2017

Well, at the very least I assume that whole JavaScript world is kind of fragmented in that respect. UMD is meant to be "one and true", but ES6 is probably even more "true", if you're talking about future. So, yeah, I guess it's easier for us to just implement them all and let the user decide (and leave UMD as a sane default that will work for 95..99% of users).

@fallenoak
Copy link
Author

Correct, I meant CommonJS earlier.

I'll verify that webpack 2 supports ES6 modules later today.

UMD is really more of a bridge format meant to deal with fragmentation until various runtimes all get native ES6 support. It's a good default, I think, but it creates unnecessary cruft for environments that support ES6 modules natively.

@fallenoak
Copy link
Author

fallenoak commented Sep 27, 2017

It looks like webpack >= 2 works just fine with ES6 modules: no Babel/etc required.

Based on this, I'd suggest we only need to support 2 or 3 possible options:

  • umd: totally fine with this remaining the default
  • es6: @koczkatamas is likely right that modern build tools will all be happy with ES6
  • none: perhaps to support unexpected cases, or cases where a postprocessor (human or otherwise) will handle adding the necessary imports / exports

And to clarify, ES6 should avoid any wrapper functions:

// any relevant import statements here

function TestKsy(_io, _parent, _root) {
  this._io = _io;
  this._parent = _parent;
  this._root = _root || this;

  this._read();
}

TestKsy.prototype._read = function() {
  // ...
}

export default TestKsy;

@GreyCat
Copy link
Member

GreyCat commented Sep 27, 2017

Do I get it correctly that, in theory, UMD has both AMD and CommonJS inside it?

@koczkatamas
Copy link
Member

koczkatamas commented Sep 27, 2017

Yes. If you run the code, then UMD works exactly the same as CJS or AMD (UMD detects which environment does it run and behaves just like that).

The problem comes when these tools don't run the code, but try to parse it to AST or I don't know and they are searching for simple require call statements. UMD confuses such parsers and they only work with vanilla CJS (at least that's my understanding on the situation).

@GreyCat
Copy link
Member

GreyCat commented Sep 27, 2017

Um, ok. So, let's do umd, es6 and none now, and if any other tool would need vanilla CommonJS and/or AMD later, we'll implement that as well.

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

No branches or pull requests

3 participants