jorgebucaran / superfine

Absolutely minimal view layer for building web interfaces
https://git.io/super
MIT License
1.56k stars 78 forks source link

Removed Rollup. Added Tsickle + Google Closure Compiler #154

Closed mini-eggs closed 5 years ago

mini-eggs commented 5 years ago

Hey! Awesome library you have here. I will probably end up using this code in a different project of mine but thought I would contribute back since I'll be modifying some code here anyways. This PR removes UglifyJS + Rollup in favor of Tsickle and GCC.

Motivations here are:

  1. End size is slightly smaller (from 3.78kB/1.66kB to 3.58kB /1.64kB).
  2. No need to manually maintain a .d.ts file.

You'll find some any and // @ts-ignore in the code. The typing isn't perfect but is good enough for Tsickle+GCC.

Downside is to build you'll now need Java installed for GCC (+ build script uses commands like gzip, and rm. I haven't tested this on MacOS but it should work (?) and probably will not work on Windows). Given that I understand not accepting this PR but thought I would send it your way regardless.

mini-eggs commented 5 years ago

I haven't tested this on MacOS but it should work

Builds fine on MacOS as far as I can tell.

jorgebucaran commented 5 years ago

@mini-eggs This looks awesome. I didn't know switching to GCC would be as simple as this. I'm quite surprised. I'm still confused as to why we need Tsickle.

While I wouldn't migrate all my projects to GCC (though I'd love to drop Rollup), I think that it makes sense to migrate Superfine and Hyperapp to GCC.

cc @frenzzy @zaceno @mindplay-dk @maxholman

mini-eggs commented 5 years ago

@jorgebucaran

I'm still confused as to why we need Tsickle.

Tsickle is really the bread and butter here! I learned of it recently and it blew me away. You'll notice two interfaces have the declare keyword prior to them

declare interface SuperFineEvent {
  currentTarget: SuperFineElement;
  type: string;
}

Tsickle will output proper JSDoc to ensure those properties will not be aggressively renamed by GCC.

While I wouldn't migrate all my projects to GCC (though I'd love to drop Rollup), I think that it makes sense to migrate Superfine and Hyperapp to GCC

Awesome to hear. If you need help migrating Hyperapp to Tsickle/GCC I'd love to help.

jorgebucaran commented 5 years ago

@mini-eggs Inquiry: did you write the TypeScript by hand or was it auto-generated?

jorgebucaran commented 5 years ago

@mini-eggs One more thing. I understand it may not be your intention here, but I'm curious, assuming one doesn't care about TypeScript, would it be possible to drop Tsickle too? Like, what if I just want GCC.

jameswilddev commented 5 years ago

According to this there's a JavaScript version of GCC (assuming that's what's being discussed here) but whether it's practical comes down to whether you're using the flags it provides: https://www.npmjs.com/package/google-closure-compiler#javascript-version

jorgebucaran commented 5 years ago

To be clear, I'd like to replace Rollup with GCC, no rewrite anything in TypeScript.

mini-eggs commented 5 years ago

@jorgebucaran

did you write the TypeScript by hand or was it auto-generated?

I wrote it.

would it be possible to drop Tsickle too?

Sure can. Instead of writing an interface like

interface SuperFineNode {
  key: any;
  element: SuperFineElement;
  type: number;
  children: Array<SuperFineNode>;
  props: SuperFineNodeProps;
  name: string;
}

You'd be doing

/**
 * @record
 */
function SuperFineNode() { }
if (false) {
    /** @type {?} */
    SuperFineNode.prototype.key;
    /** @type {!SuperFineElement} */
    SuperFineNode.prototype.element;
    /** @type {number} */
    SuperFineNode.prototype.type;
    /** @type {!Array<!SuperFineNode>} */
    SuperFineNode.prototype.children;
    /** @type {!SuperFineNodeProps} */
    SuperFineNode.prototype.props;
    /** @type {string} */
    SuperFineNode.prototype.name;
}

Then you would consuming that "type" like

/** @type {function(!SuperFineNode, !Array<!Function>, boolean): !SuperFineElement} */
var createElement = function (node, lifecycle, isSvg) {
  ...
};

The prefixed ! is what will makes GCC not rename props. (Edit: this is wrong, see below)

Personal opinion of course but writing JSDoc seems like a lot of noise + I favor editor integrations w/ TypeScript over JSDoc.

mini-eggs commented 5 years ago

Not familiar with the if(false) { ... } in the code snippet above. I imagine that's a quirk of Tsickle? Anyways more info here.

And uh, side note, something I'm confused by is how to properly export libraries with GCC. Seems no configuration of GCC flags + use of @export in JSDoc can create something UMD/CommonJS/ESModule-y.

At the bottom of src/index.ts you'll find this hack

/**
 * "Exporting" with Google Closure Compiler
 */
// @ts-ignore
self["h"] = h;
// @ts-ignore
self["patch"] = patch;
// @ts-ignore
self["recycle"] = recycle;
export default { h, patch, recycle };

Which I'm pretty unhappy with.

Edit: wait I may have some incorrect information here about exporting/GCCs renaming props. I'll post back soon.

mini-eggs commented 5 years ago

Alrighty, found out some better ways to do things w/ GCC.

  1. To have prop names not be renamed this is the proper syntax
// src/main.js
/**
 * @record
 */
function message() {}

/**
 * @type {number}
 */
message.prototype.charlieXCX;

/**
 * @type {string}
 * @export
 */
message.prototype.msg;

/**
 * @type {message}
 * @export
 */
var item = {
  charlieXCX: 1233, // this prop will be renamed
  msg: "here we go" // this one will not be renamed
};
  1. To use the @export annotation there you'll either have to use the Closure JS library or implement your own goog.exportSymbol. Here's a simple example of implementing our own
// src/base.js
/**
 * @const
 */
var goog = goog || {};

/**
 * @param {string} publicPath
 * @param {*} object
 */
goog.exportSymbol = function(publicPath, object) {
  if (typeof module !== "undefined") {
    module["exports"] = object;
  } else {
    window[publicPath] = object;
  }
};

Not sure of a better way to export in Tsickle would be but doing this with plain JS makes more sense to me now (albeit the types are slightly more annoying to write).

Edit: and the compiler options to build

java -jar ./node_modules/google-closure-compiler/compiler.jar --generate_exports --export_local_property_definitions --compilation_level ADVANCED_OPTIMIZATIONS --js_output_file=dist/main.js 'src/**.js'
mindplay-dk commented 5 years ago

@mini-eggs why did you replace the entire .d.ts file? there's a bunch of stuff missing or inaccurate in the new version - I don't understand what that has to do with the other changes you're proposing.

Rather than replacing the typings, can we progressively move them ahead with any changes/additions and maintain proper history of the file, please?

Thanks :-)

mini-eggs commented 5 years ago

@mindplay-dk

why did you replace the entire .d.ts file? there's a bunch of stuff missing or inaccurate in the new version - I don't understand what that has to do with the other changes you're proposing.

The original thought was to let Tsickle generate the .d.ts file in this PR. Seems like we don't want Tsickle so not much point in that (but yea, the typing in this PR wasn't very good my bad there).

Also, given @jorgebucaran's comment

To be clear, I'd like to replace Rollup with GCC, no rewrite anything in TypeScript.

I think I will close out and cancel this PR. I got a GCC compiling Superfine over on this repo (was just testing it out -- typing isn't perfect yet over there either) but would like two do two things in a follow up PR:

(1) Write GCC accepted JSDoc for the code here in Superfine and (2) Export two different builds of Superfine (an ES5 build and an ES6+ build like how I'm doing within the repo above).

Would you folks accept a PR with both of these?

oh and

Rather than replacing the typings, can we progressively move them ahead with any changes/additions and maintain proper history of the file, please?

I'll let the d.ts be in the follow up PR. :+1:

jorgebucaran commented 5 years ago

@mini-eggs Thank you for everything. I've been wanting to experiment with GCC for a while now and this was the first PR that showed me something concrete. 🙌

I don't know if I'll be accepting a follow-up PR after seeing how weird the JSDoc comments can get, but I'd like to see it first, then make up my mind.

I'll let the d.ts be in the follow up PR.

Feel free to remove it if you need to for GCC to work. Just saying, don't let it be a burden.

mini-eggs commented 5 years ago

@jorgebucaran Hey, thanks for the awesome lib. Made my lil Wigly library cake to implement. :-)

N cool, I'll submit another PR within the next few days.

after seeing how weird the JSDoc comments can get

Still learning the whole JSDoc thing, I'll see what I can do!