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

Rollup integration #48

Merged
merged 1 commit into from
Feb 7, 2017
Merged

Rollup integration #48

merged 1 commit into from
Feb 7, 2017

Conversation

maraisr
Copy link
Contributor

@maraisr maraisr commented Feb 6, 2017

No description provided.

@tunnckoCore
Copy link

You can remove babelrc and babel devDep. It is not used.

As about the rest, seems good workaround. I'm working on @rolldown (built on Rollup) which tries to fix some things and adds more features. So in the near days we can use rolldown cli with more simpler config. :)

@maraisr
Copy link
Contributor Author

maraisr commented Feb 6, 2017

@tunnckoCore no it is used, Jest needs it.

@tunnckoCore
Copy link

damn.. ;d

@SkaterDad
Copy link
Contributor

Looks like this saved a few hundred bytes per minified/gzipped file? 👍

@maraisr
Copy link
Contributor Author

maraisr commented Feb 6, 2017

@SkaterDad thanks for the suggestion.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Feb 7, 2017

@maraisr Something is off.

MODULENAME OUTPUTFILE                SIZE  
app        ./dist/app.min.js         3.01kB
app        ./dist/app.min.js.gz      1.37kB
html       ./dist/html.min.js        276B  
html       ./dist/html.min.js.gz     198B  
h          ./dist/h.min.js           246B  
h          ./dist/h.min.js.gz        177B  
hyperapp   ./dist/hyperapp.min.js    3.26kB
hyperapp   ./dist/hyperapp.min.js.gz 1.49kB

html.min.js doesn't make sense or rollup is using some alien gzip technology from the future.

EDIT: hyperx is not included in the bundle, just h and the call to require "hyperx".

var html = function() {
    "use strict";
    var t = function(r, a, n) {
            a.ns = "http://www.w3.org/2000/svg", n.forEach(function(r) {
                r.data && t(r.tag, r.data, r.tree)
            })
        },
        r = function(r, a, n) {
            return "svg" === r && t(r, a, n), {
                tag: r,
                data: a || {},
                tree: [].concat.apply([], n)
            }
        },
        a = require("hyperx"),
        n = a(r);
    return n
}();

@jorgebucaran
Copy link
Owner

jorgebucaran commented Feb 7, 2017

It seems rollup does not bundle non-ES6 modules without a plugin.

@SkaterDad
Copy link
Contributor

@maraisr
Copy link
Contributor Author

maraisr commented Feb 7, 2017

@SkaterDad already using that mate...

@jbucaran fixed mate.. Rollup doesnt accept the require syntax, so just converted it to a import block.

This looks alot better

app        ./dist/app.min.js         3.01kB
app        ./dist/app.min.js.gz      1.37kB
html       ./dist/html.min.js        4.31kB
html       ./dist/html.min.js.gz     1.81kB
h          ./dist/h.min.js           246B
h          ./dist/h.min.js.gz        177B
hyperapp   ./dist/hyperapp.min.js    7.29kB
hyperapp   ./dist/hyperapp.min.js.gz 3.05kB

@terkelg
Copy link
Contributor

terkelg commented Feb 7, 2017

By switching to ES2015 import/export syntax, we break node support and potential future isomorphic rendering. It will no longer be possible to run the project without using build tools.

I feel this conflicts with the core principles of HyperApp. The idea behind using Hyperx to turn template literals into vNodes is to avoid unnecessary build tools, and take advantage of built in language features for quick prototyping. (Compared to JSX which requires transpiling).

Now that the project require more complex build tools (transpile etc) we might as well switch to use JSX and remove the only dependency left. However, in my opinion that's the wrong direction.

@SkaterDad
Copy link
Contributor

Rollup can output multiple formats, so in addition to the browser builds, a Node compatibile build could be made also.

Maybe check how Vue set up their build? I would guess that codebase is full ES2015, but they support SSR.

@maraisr
Copy link
Contributor Author

maraisr commented Feb 7, 2017

@terkelg what you fail to understand is, that this is purely for cdn purposes. To have a build dist ready to go for quick prototyping... In real production use cases you'd use the import syntax as its spec, to ecma. And you'd end up using yo-yoify for template literals or jsx all directly yo h calls.

Although not natively supported, it will be in the future (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import).

And IMO its a way better syntax than that of require..

@maraisr
Copy link
Contributor Author

maraisr commented Feb 7, 2017

@SkaterDad sure - technically speaking we could produce a "node" version as well. Probably best we do that. Seeing as we do have a few optimizers and things going on.

@jorgebucaran
Copy link
Owner

@maraisr Thanks for this PR. I'll take over now and see if I can get a simpler configuration, using rollup, but creating a UMD bundle.

@jorgebucaran jorgebucaran self-requested a review February 7, 2017 03:49
@jorgebucaran
Copy link
Owner

jorgebucaran commented Feb 7, 2017

@terkelg I agree with you 💯, but if we can generate the same bundle we previously had using browserify, but smaller, what's to lose? Sure, we introduce a compilation step in this repo, but that's a burden we authors surely can bear.

  • Those of us that want to use go on and use tagged literals + HyperApp will bundle our apps using browserify -t hyperxify or yoyofy, etc., with the rollup generated UMD hyperapp.min.js.

  • Those of us who want to use JSX + HyperApp will bundle our apps using webpack or rollup with the rollup generated UMD app.min.js and h.min.js or perhaps we can roll out a hyperapp.lean.min.js or something like that.

  • Or rather, hyperapp.min.js with only app and h and hyperapp.html.min.js with everything.

@maraisr
Copy link
Contributor Author

maraisr commented Feb 7, 2017 via email

@maraisr
Copy link
Contributor Author

maraisr commented Feb 7, 2017 via email

@jorgebucaran
Copy link
Owner

jorgebucaran commented Feb 7, 2017

@maraisr Nothing. I'm using this as a base of course.

EDIT: I am just trying to see if it's possible to remove the build.js in favor of npm script only solution. Just playing.

@terkelg
Copy link
Contributor

terkelg commented Feb 7, 2017

Sure I get your point and I perfectly understand that. It's just that there's been added a lot of complexity and extra dependencies over the last two days with very little benefits.

If the only thing needed was a UMD build, it could have been done by appending a single browserify transform to the build command.

@terkelg
Copy link
Contributor

terkelg commented Feb 7, 2017

UMD is a bundle for production which definitely should be a of the build process and It's perfect for consumers. We can even host a CDN directly from NPM using https://unpkg.com/.
I totally agree that it's a good idea to provide different builds for end consumers.

However, that was possible two days ago too with Browserify. It was just a matter of adding a new line to package.json called build:umd for example and use a Browserify transform for that.

It wasn't necessary to convert the whole code base to ES2015, break node support, remove the possibility to use hyperxify out of the box and add extra dev dependencies.

(Edit: I'm not talking about the consumer, but developer experience on this project)

@tunnckoCore
Copy link

tunnckoCore commented Feb 7, 2017

@terkelg what extra deps? All changes are just completely for the development flow and improving HyperApp and some automation.

The whole thing that we should do is

  1. dist/hyperapp.es.js - pkg.module - points to the ES2015 bundle (rollup's es format) - not minified
  2. dist/hyperapp.umd.js - pkg.main - points to the UMD bundle (rollup's umd format) - not minified

It is a bit and kinda tricky. The iife format is the smallest, but it is not cool and not flexible. So best is the umd format for both browser and node.

The umd is must for few reasons.

  1. If you expose iife format to pkg.main it is good for Unpkg CDN, but not for browserify, nor nodejs
  2. If you expose iife format to pkg.browser - okey for unpkg and node, but not for browserify
  3. If above (2) and umd format to pkg.main, then it is okey for node and unpkg, but it is not clear what browserify will get (I just don't know, didn't dive deep)

So best is UMD on pkg.main and don't set pkg.browser in anyway - just for sure. Because if you set umd on pkg.main, es on pkg.module and iife on pkg.browser, then when someone use browserify over require('hyperapp') he will get the iife, which won't work.

Just played that game today, while building https://github.com/tunnckoCore/gibon

edit:

We can even host a CDN directly from NPM using https://unpkg.com/.

@terkelg yep

@maraisr
Copy link
Contributor Author

maraisr commented Feb 7, 2017

@terkelg if you want to be stuck in 2015 then sure, stick to those. But for the rest of us wanting to use modern technologies, and code using modern languages. Then yeah rollup / es2015+ would be the way to go.

@tunnckoCore
Copy link

Ooh.. and on more thing. The IIFE bundle is good just for the purpose to see REAL bytes (minus the sourcemap comment ~30-40bytes) on app, output that info and remove the bundle.

@tunnckoCore
Copy link

EDIT: I am just trying to see if it's possible to remove the build.js in favor of npm script only solution.

@jbucaran as about that.. no, it's best possible currently. Maybe we can remove zopfli and let it be as before few days. Or just that bash

_compress () {
  zopfli -h 2>/dev/null
  if [ $? -eq 0 ]; then
    zopfli "$1" -i1000 -c
  else
    gzip -c "$1"
  fi
}

build_gz () {
  _compress "dist/$1.min.js" > "dist/$1.gz"
}

build_gz "$@"

btw, @maraisr why you use zlib compression instead of gzip in the zopfli?

@terkelg
Copy link
Contributor

terkelg commented Feb 7, 2017

I think it's a healthy discussion, no reason to be harsh @maraisr. I'm fine with ES2015, and I use both that and Rollup in some of my own projects. I also think now that it's included that we should go all in and make use of stuff like jsnext:main and cjs:main in the package.json.

But I'm not a fan of converting every single function to arrow functions just because you can, and thereby make the code more cryptic for new developers. ES6 arrow functions are not even about saving keystrokes or readability, but lexical scope. They remove some of the dynamic features of the language. Functions that need a lexical name identifier self-reference (recursion, etc.) is not possible now since everything is converted to arrow functions without thoughtful consideration.

I know how to use the build tools and the advantages/disadvantages by ES6, but what about new developers? ES6 only increases your burden because it gives you more tools which you will need to learn to properly contribute to HyperApp.

@tunnckoCore
Copy link

But I'm not a fan of converting every single function to arrow functions just because you can

I mentioned it #38 (comment)

which you will need to learn to properly use HyperApp.

Anyone can use it with just include the old school script tag or require. I can't get what they will need to learn.

Multiple builds are just plus. And for Rollup is just enough to see export default, so it wasn't need the whole convert to ES2015.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Feb 7, 2017

@terkelg ES6 only increases your burden because it gives you more tools which you will need to learn to properly use HyperApp.

I don't understand what you mean. Users will consume the UMD bundle, either grabbing the CDN or on node. They are not forced to write ES6 anywhere.

@terkelg
Copy link
Contributor

terkelg commented Feb 7, 2017

@jbucaran You're right, and UMD is just that - a final build for end users. I'm not talking about end users, I'm talking about developer experience in this project. Developers interested in contributing to HyperApp or read the code to learn how a framework works. This project is perfect for that since it's so simple and elegant. That's why I'm concerned with the added complexity.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Feb 7, 2017

I've got some preliminary results of what I am thinking of, names are subject to change, but this LGTM.

@maraisr @rbiggs @terkelg @tunnckoCore @SkaterDad @danigb

7.5K hyperapp.html.min.js     # app+html+h, for prototyping
3.1K hyperapp.html.min.js.gz  # app+html+h, for prototyping
29K  hyperapp.html.min.js.map     
3.5K hyperapp.min.js     # app+h, rollup/webpack/jsx/babel/browserify+hyperxify
1.6K hyperapp.min.js.gz  # app+h, rollup/webpack/jsx/babel/browserify+hyperxify
13K  hyperapp.min.js.map

@jorgebucaran
Copy link
Owner

@terkelg Thank your for clarifying, now I see what you meant. My changes over this PR will drastically reduce the complexity, so hang on.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Feb 7, 2017

@maraisr Can you create the branch on this repo instead of your fork? That way we can both push to the same branch, otherwise I'd have to send a PR to your fork.

EDIT: We try that in the future. I'll push my changes to a new branch and create a PR so you can review it.

@maraisr maraisr changed the base branch from master to feature/rollup February 7, 2017 11:01
@maraisr maraisr merged commit e529e86 into jorgebucaran:feature/rollup Feb 7, 2017
@maraisr maraisr deleted the feature/rollup branch February 7, 2017 11:01
@maraisr
Copy link
Contributor Author

maraisr commented Feb 7, 2017

There is another branch sitting on this repo. @jbucaran

@jorgebucaran
Copy link
Owner

@maraisr I am still at work. When I get off I'll start wrapping this up for the 0.0.12 release.

@maraisr
Copy link
Contributor Author

maraisr commented Feb 7, 2017

Thank you again @terkelg we will take your input into consideration.

@maraisr
Copy link
Contributor Author

maraisr commented Feb 7, 2017

@jbucaran cool mate - let me have a look before we merge. Tend to agree with @terkelg we do overuse the consts / arrow functions. Anyway... All this "arguing" is doing my head in, it's all very subjective, and with my experience with OSS everyone has their opinions of how things ought to be done, which is fair enough...

But we just got to realize out of all of this - we're all humans, we all come from different parts of the world, Australia, Japan, and America! All three have different trends floating around in that part of the globe.

And this lib being in its infancy, it doesnt matter how "solid" we have things just yet. I mean Vue was re-written, Webpack was re-written, Angular is in its 3rd phase of a re-write. So like, just pick a bloody bundler and get it over and done with.

The only thing I'll stand by and argue is - this lib is going to be written in es6+. This "cryptic" and nonsense around "new developers" is all just garbage. Almost all of the latest tutorial / help forums these days are all es6+ examples.

Anyway. Lets keep things positive boys.

This was referenced Feb 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants