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

Removed Rollup. Added Tsickle + Google Closure Compiler #154

Closed
wants to merge 3 commits into from

Conversation

mini-eggs
Copy link

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
Copy link
Author

mini-eggs commented Nov 3, 2018

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

Builds fine on MacOS as far as I can tell.

package.json Outdated
"jest": "^23.3.0",
"rollup": "^0.62.0",
"uglify-js": "^3.4.3"
"npm-run-all": "^4.1.3",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mini-eggs We don't need npm-run-all do we?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

.babelrc Outdated
@@ -0,0 +1,13 @@
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mini-eggs What do we need babel for?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how this got in here... Must have been auto gen'd by something. I was playing with Parcel while dev'ing -- maybe that snuck it in?

@jorgebucaran jorgebucaran added the enhancement New feature or request label Nov 4, 2018
@jorgebucaran
Copy link
Owner

@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
Copy link
Author

mini-eggs commented Nov 4, 2018

@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
Copy link
Owner

jorgebucaran commented Nov 4, 2018

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

@jorgebucaran
Copy link
Owner

@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
Copy link

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
Copy link
Owner

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

@mini-eggs
Copy link
Author

mini-eggs commented Nov 4, 2018

@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
Copy link
Author

mini-eggs commented Nov 4, 2018

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
Copy link
Author

mini-eggs commented Nov 4, 2018

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
Copy link
Contributor

@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
Copy link
Author

@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. 👍

@mini-eggs mini-eggs closed this Nov 6, 2018
@jorgebucaran
Copy link
Owner

@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.

@jorgebucaran jorgebucaran added discussion Refactor and removed enhancement New feature or request labels Nov 6, 2018
@mini-eggs
Copy link
Author

@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!

@mini-eggs mini-eggs deleted the tsickle+gcc branch November 9, 2018 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants