Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

No more minified assets in npm distribution? #5522

Closed
scottohara opened this issue Feb 22, 2016 · 8 comments
Closed

No more minified assets in npm distribution? #5522

scottohara opened this issue Feb 22, 2016 · 8 comments

Comments

@scottohara
Copy link

Bug description:

Previously angular-ui-bootstrap@0.14.2 (installed via npm) included minified assets as part of the distribution:

node_modules/angular-ui-bootstrap/ui-bootstrap.min.js
node_modules/angular-ui-bootstrap/ui-bootstrap-tpls.min.js

Current angular-ui-bootstrap@1.1.2 moves the assets to a dist/ directory, but no longer includes minified versions:

node_modules/angular-ui-bootstrap/dist/ui-bootstrap.js
node_modules/angular-ui-bootstrap/dist/ui-bootstrap-tpls.js

Was this a conscious decision to stop distributing minified versions in the latest npm packages?

I ask not because it would be difficult to minify these scripts myself (I'm already minifying my own assets as part of a build process). Rather, this change puts angular-ui-bootstrap out of step with every other 3rd party package included my app, each of which includes minified assets as part of their official distributions:

// npm install jquery@2.2.0
// Includes minified assets? ✔
node_modules/jquery/dist/jquery.min.js

// npm install bootstrap@3.3.6
// Includes minified assets? ✔
node_modules/bootstrap/dist/js/bootstrap.min.js

// npm install angular@1.5.0
// Includes minified assets? ✔
node_modules/angular/angular.min.js"

// npm install angular-ui-router@0.2.18
// Includes minified assets? ✔
node_modules/angular-ui-router/release/angular-ui-router.min.js

// npm install angular-ui-bootstrap@1.1.2
// Includes minified assets? ✖
node_modules/angular-ui-bootstrap/dist/ui-bootstrap.js
node_modules/angular-ui-bootstrap/dist/ui-bootstrap-tpls.js

// npm install moment@2.11.2
// Includes minified assets? ✔
node_modules/moment/min/moment.min.js

// npm install babel-core@5.8.25
// Includes minified assets? ✔
node_modules/babel-core/browser-polyfill.min.js

Please consider reintroducing minified assets into the official npm distribution, if only for consistency with other popular npm packages.

Link to minimally-working plunker that reproduces the issue:

N/A

Version of Angular, UIBS, and Bootstrap

Angular: 1.5.0

UIBS: 1.1.2

Bootstrap: 3.3.6

@deeg deeg added this to the 1.2.0 milestone Feb 22, 2016
@deeg
Copy link
Contributor

deeg commented Feb 22, 2016

Thanks for posting this. Was not on purpose and will be re-added.

Edit: I stand corrected, it was done on purpose.

After talking with team it seems like it was removed due to most people not using them since people tend to minify their own projects.

Do you have a strong reason to want it back other than most libraries do it? Do you not minify your project files?

@deeg deeg removed this from the 1.2.0 milestone Feb 22, 2016
@scottohara
Copy link
Author

Yes, the main reason I would like to see it returned is for consistency with other libraries.

This isn't just me being a purist, though.

In my project, I have:

  • gulp tasks that process my project files (build:app:js, clean:app:js, ...)
  • gulp tasks that process 3rd party libraries (build:vendor:js, clean:vendor:js, ...)

These two classes of assets have different processing requirements.

For example, my own assets are: concatenated, transpiled from ES2015 (babel), minified, fingerprinted, and source mapped:

// Build application Javascript
const appJsSource = "src/**/*.js";

gulp.task("build:app:js", ["clean:app:js"], () => gulp.src(appJsSource)
    .pipe(sourceMaps.init())
        .pipe(size({title: "app js (original)"}))
        .pipe(concat("app.js"))
        .pipe(babel({presets: ["es2015"]}))
        .pipe(uglify())
        .pipe(rev())
        .pipe(size({title: "app js (minified)"}))
        .pipe(size({title: "app js (gzipped)", gzip: true}))
    .pipe(sourceMaps.write("."))
    .pipe(gulp.dest("public"))
    .on("error", util.log));

3rd party assets have (at least until now) typically been distributed as ES5 and already minified (often, but not always, with source maps); so there are fewer processing steps required for those:

// Build vendor Javascript
const vendorJsSource = [
    "node_modules/jquery/dist/jquery.min.js",
    "node_modules/bootstrap/dist/js/bootstrap.min.js",
    "node_modules/angular/angular.min.js",
    "node_modules/angular-ui-router/release/angular-ui-router.min.js",
    "node_modules/angular-ui-bootstrap/dist/ui-bootstrap.js",
    "node_modules/angular-ui-bootstrap/dist/ui-bootstrap-tpls.js",
    "node_modules/moment/min/moment.min.js",
    "node_modules/babel-polyfill/browser.js"
];

gulp.task("build:vendor:js", ["clean:vendor:js"], () => gulp.src(vendorJsSource)
    .pipe(sourceMaps.init({loadMaps: true}))
        .pipe(concat("vendor.js"))
        .pipe(rev())
    .pipe(sourceMaps.write("."))
    .pipe(gulp.dest("public"))
    .on("error", util.log));

You could certainly argue that adding an uglify() step into the build:vendor:js pipeline would solve this problem, and I agree it would.

But it seems (to me) wrong to require all users of your library to do their own minification when other libraries don't. It seems a rather arbitrary choice.

Another (albeit lesser) reason for including minified assets would be to ensure that most users are running unmodified official code. I wonder how many issues might be raised in future that are in fact problems introduced by different compression tools/versions (eg. YUI compressor, jscompress, uglify etc.)?

I haven't used enough of these tools to know whether it is possible for one to introduce a bug/problem into the minified code; but I can certainly imagine that if that were the case, you may see issues logged here as a result. Such issues could be easily triaged by asking whether it can reproduced on the unminified version; but providing official minified versions would avoid these potential issues entirely.

@deeg deeg removed the needs: info label Feb 23, 2016
@wesleycho
Copy link
Contributor

It seems dirty to include it when a large portion, if not most, using npm are likely also using CommonJS as well. It adds overhead to whole npm install process, and if many libraries do so, it makes it even slower.

I'm willing to revisit that decision, but can't say I like encouraging people to not concat and minify their assets. WDYT @Foxandxss ?

@Foxandxss
Copy link
Contributor

I can see both opinions to be valid.

I think that libraries are shifting to not give .min versions. Nowadays you do a npm install foo and then import foo from 'foo' and not caring about what file is it exactly loading, you just import it. Then your tooling will make sure to uglify that for you, no hassle.

The other way is when you cherry pick what you need, with that you directly can't use a .min

For older workflows (no require), I think that it is a good practice to have a uglifier. At the end you need to uglify your app, so you can uglify everything. I prefer that against uglifying my app and then concat third party .min files.

@scottohara
Copy link
Author

Thanks for your responses, @wesleycho & @Foxandxss.

My app uses an old-style workflow (not using modules/bundling with Browserify/Webpack/Systemjs), but I see your point that in modern, module-based bundling; .min assets no longer have as much value.

I'm not entirely sure I follow the line of thinking that including minified versions is

encouraging people to not concat and minify their assets

If it were up to me, I would include both minified and non-minified versions; to support both old and new use cases.

If the only real objection to restoring the minified versions would be a slight slowdown in npm install times, I (personally) don't see that as reason enough to exclude them. (I'd be surprised if the overhead was even noticeable).

@wesleycho
Copy link
Contributor

I'm not hugely against including the minified versions again, and sort of anticipated this issue.

I guess this comes down to our intentions - @Foxandxss what are your thoughts? Those not using TS/ES6/commonjs probably see this problem more acutely.

@wesleycho
Copy link
Contributor

After discussion with the team, I'm closing this as something we aren't interested in supporting. We understand the potentially pain with older workflows, but we encourage more modern practices with the use of a tool for concating and minifying vendor assets.

@scottohara
Copy link
Author

Fair enough. Thanks for clarify the project's intentions.

gmcharlt pushed a commit to evergreen-library-system/Evergreen that referenced this issue Apr 12, 2017
Per angular-ui/bootstrap#5522 upstream
feels users should minimize the files themselves.

Signed-off-by: Dan Scott <dan@coffeecode.net>
Signed-off-by: Bill Erickson <berickxx@gmail.com>
Signed-off-by: Ben Shum <ben@evergreener.net>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants