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

Paglias node package #866

Merged
merged 14 commits into from
Mar 11, 2013
Merged

Paglias node package #866

merged 14 commits into from
Mar 11, 2013

Conversation

SteveSanderson
Copy link
Contributor

No description provided.

@SteveSanderson SteveSanderson mentioned this pull request Feb 27, 2013
@SteveSanderson
Copy link
Contributor Author

This branch is where I just published https://npmjs.org/package/knockout from.

Is everyone happy with the NPM module itself? It certainly seems to work OK when I just tried it.

Michael/Ryan - are you happy with these modifications to the KO repo? If so please go ahead and merge whenever you're ready!

@paglias
Copy link
Contributor

paglias commented Feb 27, 2013

I understand not putting the minified version but seen how Knockout is minified this causes a lot of problems.

Actually Knockout is written in a way to be optimized when using google closure compiler (ko.exportSymbol, referencing to functions using ko["function"], ....)

And this is ok as long as we use the version minified using the closure compiler that create a 40kb version. Instead when using another service like UglifyJS it comes out a 58kb version a lot of which could be reduced removing things specific to the closure compiler.

Let's say I want to use the npm package with browserify or similar tools, I'll get up a compiled version a lot bigger because it's written to be used with closure compiler.

I think that for now the best thing would be to keep the minified version, then in the future:

1-) Publish an unminified version without the DEBUG stuff, this is necessary not because DEBUG slow down Knockout but to provide a consistent behavior: the latest value should be always accessible or if you don't want to include it it should never be (you can subscribe to beforeChange if you need it)

2-) Remove the optimizations for the closure compiler so that it can be used with any minifier out there. For private APIs we could create a different namespace or use another technique

@paglias
Copy link
Contributor

paglias commented Feb 27, 2013

Another thing, I saw that you removed the node scripts:

1-) the test script will also work on window so we can keep it
2-) for the build script, I know it would be a big change, what about using a node based build tool that will work cross-platform like Grunt.js that is actively maintained, has a lot of plugins and widely used? Yes, it will require Node to be installed but I think that more or less every javascript developer use it.

@mbest
Copy link
Member

mbest commented Feb 27, 2013

I've re-added the "scripts" section so that npm can build Knockout, with modifications so that the build should work in any environment.

I also think it makes more sense to use the minified build of Knockout in the package since that's the official version.

@domenic
Copy link

domenic commented Feb 28, 2013

+1 to @mbest's changes.

I'd prefer the unminified version since to me the extra 18kb is totally worth the seamlessness of being able to treat Knockout line any other script in my build process. But I see the arguments for the minified version (and recognize Knockout has some unique, if unfortunate, constraints there). One compromise might be to make the unminified version the default but use the browser field in package.json to tell browserify and any others that follow this de-facto spec that they should use the minified version.

@SteveSanderson
Copy link
Contributor Author

Thanks for fixing the scripts Michael! That is much better now it works in all environments.

[paglias] Remove the optimizations for the closure compiler so that it can be used with any minifier out there.

The Closure Compiler optimizations are really valuable from the point of view of minimizing the library size for in-browser use. I think we would consider removing them and accepting the use of arbitrary minifiers only if it was possible to achieve the same level of size reduction. Right now I'm not aware that any minifier is even in the same league as Closure Compiler if you're willing to optimize your code style for that minifier. If we could get uglifyjs to produce results that good then I'd be very happy to switch (it would simplify the build process), but I would have to see it to believe it.

For private APIs we could create a different namespace or use another technique

In the long run it would be nice to avoid the hard dependency on minification as a way of differentiating public/private APIs. Then at least it would be valid for people to use arbitrary minifiers even if the results aren't as compact. Possible techniques would include:

  • Hiding private APIs within the closure. Basically, instead of attaching private stuff to ko, we attach it to internal or something, and then don't export internal publicly. This only works for top-level APIs, and not for functions or properties assigned to objects that must themselves be exposed (e.g., instances of ko.observable).
  • Naming convention. Simply prefixing private property/function names with an underscore would be a pretty clear indication that we don't expect people to reference those ones from external code. The minified result would be identical. If someone chooses to rely on an underscored property/function then it's their risk: we don't support it or document it, and that code is subject to break in every version update. For fun, we may even add a prebuild step that adds random suffixes to underscored symbols just to make it impractical to reference them from external code :)

Anyway, this is a separate discussion and is not a high priority right now.

[mbest] I also think it makes more sense to use the minified build of Knockout in the package since that's the official version.
[domenic] I'd prefer the unminified version

Uh... hmm... well. Not sure how to break the deadlock here. I'm happy either way if we can reach consensus. I'm open to the idea that the cultural norms for Node development are not necessarily identical to those for in-browser JS development, and therefore that we could have a different policy about which file to use. Ultimately the whole project's objective is to be useful to the people who use it, so in this case what is most useful for Node developers? Which of us has enough familiarity with Node culture to make that call? I use Node a fair bit in my work but still think of myself as something of a noob and would happily defer to a Node insider on this. I can see how it would seem a little alien to be debugging a Node app with Node Inspector and suddenly step into minified code. Without knowing KO in detail, you would be totally baffled about why code on the server has been minified. But would it be acceptable even so?

Michael, do you think Domenic's suggestion about the browser field in package.json is a good solution?

Domenic, do you think Node devs could accept minified code without being shocked and confused?

@paglias
Copy link
Contributor

paglias commented Feb 28, 2013

On the minification, I understand it would be a big change and I don't expect it to happen because there are more important things, but:

Today I played a bit with the code and removed all the closure compiler optimizations (private members prefixed with an underscore) and I came out with a 49kb version that reduce to 14.72 when gzipped (vs 14.50kb of the official minified build. Would a build of this size be acceptable?

@SteveSanderson
Copy link
Contributor Author

@domenic and @mbest, if I could ping you to find out how you think we should resolve the minified-or-not-in-node question, then I'll merge this into master!

Today I played a bit with the code and removed all the closure compiler optimizations (private members prefixed with an underscore) and I came out with a 49kb version that reduce to 14.72 when gzipped (vs 14.50kb of the official minified build. Would a build of this size be acceptable?

That's cool - thanks for investigating! We probably wouldn't want the official minified (pre-gzip) version to suddenly jump from 40kb to 49kb without any new features to show for it, as plenty of developers will be looking at the disk file size when reviewing and comparing options for libraries to depend on, and that's quite a change. However, we probably don't need to stop using Closure Compiler for the official minified build: we could keep using it, leaving its optimizations in place, but just change to the underscore convention for privates. Then the official minified result doesn't change at all, but it becomes legitimate for people to use alternative minifiers if they want (e.g., because they have a custom build system, and don't care so much about the extra few hundred bytes after gzipping). Such unofficial minified builds won't be as compact, but hey, it's their choice. At that point we could even switch to the .js/.min.js convention if we wanted. And the matter of what to do with the NPM package becomes obvious: we would reference the unminified version, since the file size is basically irrelevant there.

Anyway, like you say, this isn't a top priority and we probably wouldn't rush to do it. But in principle what do you think?

@mbest
Copy link
Member

mbest commented Mar 1, 2013

I think @domenic's suggestion of adding the browser field in package.json with the minified version and leaving the debug version as the main one is a good compromise.

@paglias
Copy link
Contributor

paglias commented Mar 1, 2013

I'm ok with keeping using closure compiler. On underscore prefix: using it would mean removing also exportSymbol and exportProperty? I'm not sure the build size will remain the same, tomorrow I'll try to see what happens.

For the npm package the only thing that I still don't like is the DEBUG in the unminified build: what ab out adding a ko.debug property that can be set to true when the developer need it? So that there won't be differencies beetwen the minified and unminified build?

@domenic
Copy link

domenic commented Mar 2, 2013

I'd prefer setting "main" to the unminified version, and not setting the "browser" field, since if I am using browserify it'd be weird to have all my libraries be unminified at development time except for Knockout which becomes minified. Similarly, it's a sucky debugging experience to get those stack traces full of single-letter function and method names. I only offered it as an option in case you felt it was necessary that browser users---including browserify users---get the official minified semantics.

I'd be very excited about the possibility of killing Closure Compiler's influence on the codebase altogether, but of course don't want that move to block updating the Node package. I'd like to note that most libraries these days advertise their mingzipped size, and make no mention of their minified size. E.g. jQuery says "Only 32kB minified and gzipped," Ember says "49k min+gzip", Backbone says "6.3kb, Packed and gzipped." I don't think "14.7k min+gzip" versus "14.5k min+gzip" will lose you any potential developers.

@mbest
Copy link
Member

mbest commented Mar 5, 2013

Dominic is much more knowledgeable than I am about Node.js development, so I'll defer to his advice.

On a different topic, I think we should set the version to something like 2.3.0pre so it's not confused with the actual 2.2.1.

@paglias paglias mentioned this pull request Mar 6, 2013
22 tasks
@SteveSanderson SteveSanderson merged commit c621474 into master Mar 11, 2013
@SteveSanderson
Copy link
Contributor Author

OK sounds good. I've left the package.json pointing to the unminified source, and tweaked the version in package.json to 2.3.0pre.

Overall I'm liking the direction of the discussion about changing minification style. Again, it's absolutely not a priority to be implemented soon as compared with other features, but it would be nice in the long run.

@mbest mbest deleted the paglias-node-package branch April 4, 2013 23:54
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.

None yet

4 participants