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

dist/script.[hash].js wasn't be compressed with running ng build --prod command line #2796

Closed
axetroy opened this issue Oct 20, 2016 · 11 comments · Fixed by #7279
Closed

dist/script.[hash].js wasn't be compressed with running ng build --prod command line #2796

axetroy opened this issue Oct 20, 2016 · 11 comments · Fixed by #7279
Assignees
Labels
P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@axetroy
Copy link

axetroy commented Oct 20, 2016

OS?

Mac OSX Yosemite

Versions.

angular-cli: 1.0.0-beta.17
node: 6.5.0
os: darwin x64

Repro steps.

The log given by the failure.

All pass no fail

Mention any other details that might be useful.

angular-cli.json

      "scripts": [
        "../node_modules/jquery/dist/jquery.js",
        "../node_modules/bootstrap/dist/js/bootstrap.js",
        "../node_modules/moment/moment.js"
      ],

and run command line ng build --prod

Here is a part of code about dist/script.[hash].js

webpackJsonp([2,3],{121:function(t,n){t.exports=function(t){"undefined"!=typeof execScript?execScript(t):eval.call(null,t)}},212:function(t,n,e){e(121)(e(435))},213:function(t,n,e){e(121)(e(436))},214:function(t,n,e){e(121)(e(437))},435:function(t,n){t.exports="/*!\n * Bootstrap v3.3.7 (http://getbootstrap.com)\n * Copyright 2011-2016 Twitter, Inc.\n * Licensed under the MIT license\n */\n\nif (typeof jQuery === 'undefined') {\n  throw new Error('Bootstrap\\'s JavaScript requires jQuery')\n}\n\n+function ($) {\n  'use strict';\n  var version = $.fn.jquery.split(' ')[0].split('.')\n  if ((version[0] < 2 && version[1] < 9) || (version[0] == 1 && version[1] == 9 && version[2] < 1) || (version[0] > 3)) {\n    throw new Error('Bootstrap\\'s JavaScript requires jQuery version 1.9.1 or higher, but lower than version 4')\n  }\n}(jQuery);\n\n/* ========================================================================\n * Bootstrap: transition.js v3.3.7\n * http://getbootstrap.com/javascript/#transitions\n * ========================================================================\n * Copyright 2011-2016 Twitter, Inc.\n * Licensed under MIT (https://github.com/twbs/bootstrap/blob/master/LICENSE)\n * ======================================================================== */\n\n\n+function ($) {\n  'use strict';\n\n  // CSS TRANSITION SUPPORT (Shoutout: http://www.modernizr.com/)\n  // ============================================================\n\n  function transitionEnd() {\n    var el = document.createElement('bootstrap')\n\n    var transEndEventNames = {\n      WebkitTransition : 'webkitTransitionEnd',\n      MozTransition    : 'transitionend',\n      OTransition      : 'oTransitionEnd otransitionend',\n      transition       : 'transitionend'\n    }\n\n    for (var name in transEndEventNames) {\n      if (el.style[name] !== undefined) {\n        return { end: transEndEventNames[name] }\n      }\n    }\n\n    return false // explicit for ie8 (  ._.)\n  }\n\n  // http://blog.alexmaccaw.com/css-transitions\n  $.fn.emulateTransitionEnd = function (duration) {\n    var called = false\n    var $el = this\n    $(this).one('bsTransitionEnd', function () { called = true })\n    var callback = function () { if (!called) $($el).trigger($.support.transition.end) }\n    setTimeout(callback, duration)\n    return this\n  }\n\n  $(function () {\n    $.support.transition = transitionEnd()\n\n    if (!$.support.transition) return\n\n    $.event.special.bsTransitionEnd = {\n      bindType: $.support.transition.end,\n      delegateType: $.support.transition.end,\n      handle: function (e) {\n        if ($(e.target).is(this)) return e.handleObj.handler.apply(this, arguments)\n      }\n    }\n  })\n\n}(jQuery);\n\n/* ========================================================================\n * Bootstrap: alert.js v3.3.7\n * http://getbootstrap.com/javascript/#alerts\n * ========================================================================\n * Copyright 2011-2016 Twitter, Inc.\n * Licensed under MIT (https://github.com/twbs/bootstrap/blob/master/LICENSE)\n * ======================================================================== */\n\n\n+function ($) {\n  'use strict';\n\n  // ALERT CLASS DEFINITION\n  // ======================\n\n  var dismiss = '[data-dismiss=\"alert\"]'\n  var Alert   = function (el) {\n    $(el).on('click', dismiss, this.close)\n  }\n\n  Alert.VERSION = '3.3.7'\n\n  Alert.TRANSITION_DURATION = 150\n\n  Alert.prototype.close = function (e) {\n    var $this    = $(this)\n    var selector = $this.attr('data-target')\n\n    if (!selector) {\n      selector = $this.attr('href')\n      selector = selector && selector.replace(/.*(?=#[^\\s]*$)/, '') // strip for ie7\n    }\n\n    var $parent = $(selector === '#' ? [] : selector)\n\n    if (e) e.preventDefault()\n\n    if (!$parent.length) {\n      $parent = $this.closest('.alert')\n    }\n\n    $parent.trigger(e = $.Event('close.bs.alert'))\n\n    if (e.isDefaultPrevented()) return\n\n    $parent.removeClass('in')\n\n    function removeElement() {\n      // detach from parent, fire event then clean up data\n      $parent.detach().trigger('closed.bs.alert').remove()\n    }\n\n    $.support.transition && $parent.hasClass('fade') ?\n      $parent\n        .one('bsTransitionEnd', removeElement)\n        .emulateTransitionEnd(Alert.TRANSITION_DURATION) :\n      removeElement()\n  }\n\n\n  // ALERT PLUGIN DEFINITION\n  // =======================\n\n  function Plugin(option) {\n    return this.each(function () {\n      var $this = $(this)\n      var data  = $this.data('bs.alert')\n\n      if (!data) $this.data('bs.alert', (data = new Alert(this)))\n      if (typeof option == 'string') data[option].call($this)\n    })\n  }\n\n  var old = $.fn.alert\n\n  $.fn.alert             = Plugin\n  $.fn.alert.Constructor = Alert\n\n\n  // ALERT NO CONFLICT\n  // =================\n\n  $.fn.alert.noConflict = function () {\n    $.fn.alert = old\n    return this\n  }\n\n\n  // ALERT DATA-API\n  // ==============\n\n  $(document).on('click.bs.alert.data-api', dismiss, Alert.prototype.close)\n\n}(jQuery);\n\n/* ========================================================================\n * Bootstrap: button.js v3.3.7\n * http://getbootstrap.com/javascript/#buttons\n * ========================================================================\n * Copyright 2011-2016 Twitter, Inc.\n * Licensed under MIT (https://github.com/twbs/bootstrap/blob/master/LICENSE)\n * ======================================================================== */\n\n\n+function ($) {\n  'use strict';\n\n  // BUTTON PUBLIC CLASS DEFINITION\n  // ==============================\n\n  var Button = function (element, options) {\n    this.$element  = $(element)\n    this.options   = $.extend({}, Button.DEFAULTS, options)\n    this.isLoading = false\n  }\n\n  Button.VERSION  = '3.3.7'\n\n  Button.DEFAULTS = {\n    loadingText: 'loading...'\n  }\n\n  Button.prototype.setState = function (state) {\n    var d    = 'disabled'\n    var $el  = this.$element\n    var val  = $el.is('input') ? 'val' : 'html'\n    var data = $el.data()\n\n    state += 'Text'\n\n    if (data.resetText == null) $el.data('resetText', $el[val]())\n\n    // push to event loop to allow forms to submit\n    setTimeout($.proxy(function () {\n      $el[val](data[state] == null ? this.options[state] : data[state])\n\n      if (state == 'loadingText') {\n        this.isLoading = true\n        $el.addClass(d).attr(d, d).prop(d, true)\n      } else if (this.isLoading) {\n        this.isLoading = false\n        $el.removeClass(d).removeAttr(d).prop(d, false)\n      }\n    }, this), 0)\n  }\n\n  Button.prototype.toggle = function () {\n    var changed = true\n    var $parent = this.$element.closest('[data-toggle=\"buttons\"]')\n\n    if ($parent.length) {\n      var $input = this.$element.find('input')\n      if ($input.prop('type') == 'radio') {\n        if ($input.prop('checked')) changed = false\n        $parent.find('.active').removeClass('active')\n        this.$element.addClass('active')\n      } else if ($input.prop('type') == 'checkbox') {\n        if (($input.prop('checked')) !== this.$element.hasClass('active')) changed = false\n        this.$element.toggleClass('active')\n      }\n      $input.prop('checked', this.$element.hasClass('active'))\n      if (changed) $input.trigger('change')\n    } else {\n      this.$element.attr('aria-pressed', !this.$element.hasClass('active'))\n      this.$element.toggleClass('active')\n    }\n  }\n\n\n  // BUTTON PLUGIN DEFINITION\n  // ========================\n\n  function Plugin(option) {\n    return this.each(function () {\n      var $this   = $(this)\n      var data    = $this.data('bs.button')\n      var options = typeof option == 'object' && option\n\n      if (!data) $this.data('bs.button', (data = new Button(this, options)))\n\n      if (option == 'toggle') data.toggle()\n      else if (option) data.setState(option)\n    })\n  }\n\n  var old = $.fn.button\n\n  $.fn.button             = Plugin\n  $.fn.button.Constructor = Button\n\n\n  // BUTTON NO CONFLICT\n  // ==================\n\n  $.fn.button.noConflict = function () {\n    $.fn.button = old\n    return this\n  }\n\n\n  // BUTTON DATA-API\n  // ===============\n\n  $(document)\n    .on('click.bs.button.data-api', '[data-toggle^=\"button\"]', function (e) {\n      var $btn = $(e.target).closest('.btn')\n      Plugin.call($btn, 'toggle')\n      if (!($(e.target).is('input[type=\"radio\"], input[type=\"checkbox\"]'))) {\n        // Prevent double click on radios, and the double selections (so cancellation) on checkboxes\n        e.preventDefault()\n        // The target component still receive the focus\n        if ($btn.is('input,button')) $btn.trigger('focus')\n        else $btn.find('input:visible,button:visible').first().trigger('focus')\n      }\n    })\n    .on('focus.bs.button.data-api blur.bs.button.data-api', '[data-toggle^=\"button\"]', function (e) {\n      $(e.target).closest('.btn').toggleClass('focus', /^focus(in)?$/.test(e.type))\n    })\n\n}(jQuery);\n\n/* ========================================================================\n * Bootstrap: carousel.js v3.3.7\n * http://getbootstrap.com/javascript/#carousel\n * ========================================================================\n * Copyright 2011-2016 Twitter, Inc.\n * Licensed under MIT (https://github.com/twbs/bootstrap/blob/master/LICENSE)\n * ======================================================================== */\n\n\n+function ($) {\n  'use strict';\n\n  // CAROUSEL CLASS DEFINITION\n  // =========================\n\n  var Carousel = function (element, options) {\n    this.$element    = $(element)\n    this.$indicators = this.$element.find('.carousel-indicators')\n    this.options     = options\n    this.paused      = null\n    this.sliding     = null\n    this.interval    = null\n    this.$active     = null\n    this.$items      = null\n\n    this.options.keyboard && this.$element.on('keydown.bs.carousel', $.proxy(this.keydown, this))\n\n    this.options.pause == 'hover' && !('ontouchstart' in document.documentElement) && this.$element\n      .on('mouseenter.bs.carousel', $.proxy(this.pause, this))\n      .on('mouseleave.bs.carousel', $.proxy(this.cycle, this))\n  }\n\n  Carousel.VERSION  = '3.3.7'\n\n  Carousel.TRANSITION_DURATION = 600\n\n  Carousel.DEFAULTS = {\n    interval: 5000,\n    pause: 'hover',\n    wrap: true,\n    keyboard: true\n  }\n\n  Carousel.prototype.keydown = function (e) {\n    if (/input|textarea/i.test(e.target.tagName)) return\n    switch (e.which) {\n      case 37: this.prev(); break\n      case 39: this.next(); break\n      default: return\n    }\n\n    e.preventDefault()\n  }\n\n  Carousel.prototype.cycle = function (e) {\n    e || (this.paused = false)\n\n    this.interval && clearInterval(this.interval)\n\n    this.options.interval\n      && !this.paused\n      && (this.interval = setInterval($.proxy(this.next, this), this.options.interval))\n\n    return this\n  }\n\n  Carousel.prototype.getItemIndex = function (item) {\n    this.$items = item.parent().children('.item')\n    return this.$items.index(item || this.$active)\n  }\n\n  

You can see that, this pack wasn't be compressed

same as style

@axetroy axetroy changed the title dist/script.[hash].js wasn't compress with ng build --prod command line dist/script.[hash].js wasn't be compressed with running ng build --prod command line Oct 20, 2016
@clydin
Copy link
Member

clydin commented Oct 21, 2016

Webpack's script-loader dumps the scripts in a giant eval. This prevents any optimizations from occurring.

The use of eval is also going to cause problems for strict CSP configurations.

@filipesilva
Copy link
Contributor

filipesilva commented Oct 23, 2016

@clydin I didn't know that was why script-loader didn't optimise anything, thanks for the info!

Going over the issue tracker for script-loader I find webpack-contrib/script-loader#1 and webpack-contrib/script-loader#9, which make me think this is fixable by chaining an uglifier into script loader in prod builds.

It is also possible to just pass in the minified scripts on the scripts array, but not might be suboptimal overall.

@filipesilva filipesilva added type: enhancement P5 The team acknowledges the request but does not plan to address it, it remains open for discussion labels Oct 23, 2016
@clydin
Copy link
Member

clydin commented Oct 24, 2016

@filipesilva your welcome. Chaining an uglifier loader should fix the optimization problem but adds an additional 3rd party dependency and also doesn't solve the CSP issue. With the increasing stability of AOT support, stricter CSP configurations are becoming more viable.

@kylecordes
Copy link

In this discussion about how/whether included scripts should get minified, there is an important practical factor that I believe is worthy of getting right.

Some libraries (and in particular, of relevance to us hopefully here, Angular 1.x) ship a development build and a production/minified build, and the latter is not just the result of running the former through a minifier, but rather the result of additional, library specific differences between something intended for development versus production.

Therefore, I believe the most correct answer to this is that the configuration file should accept two lists of scripts; one for development and another for production. As application authors we can then include the path of the correct corresponding library files. This yields smaller (and sometimes faster) results than just grabbing all of the development libraries and minifying them.

As an additional convenience, it would be most ideal if there is a way to further ask CLI to minify third-party libraries which do not ship with the production build in the box.

@clydin
Copy link
Member

clydin commented Oct 24, 2016

This could be accomplished by allowing both a string and an object in the scripts setting. The object could contain production and development fields. Keeping the string allows backward compatibility as well as simplifying the typical use case.

However, some important questions to ask: how often will this be needed? And is it worth the added complexity and cost to maintain long-term?

As to minifying, why not always minify during production builds and keep things simple?

@kylecordes
Copy link

why not always minify during production builds

Certainly this keeps things simple. I think the goals of CLI include both simplicity for the application developer and high quality production-grade results. Each tradeoff that gets more of the first by sacrificing the second, pushes another increment of developers away from CLI to their own application-specific ad hoc process.

@clydin
Copy link
Member

clydin commented Oct 24, 2016

@kylecordes, my last question was not meant to suggest the lack of need for the bifurcation of the scripts option. But instead meant to be more speculative; as in, even with the addition, why not always minify production? Especially if the scripts can be bundled and optimizations can be run as a whole. But there may well be edge cases that need to be considered.

Personally and in a more general context, I've found that simplicity generally leads to higher quality production-grade results. More options leads to additional complexity which leads to a high cost toolchain and misconfigured projects. But, of course, simplicity and high quality are subjective and usually moving targets.
But i've gone off-topic. I'll leave it to @filipesilva to decide on a path forward.

@filipesilva
Copy link
Contributor

Personally I think the most practical avenue is to always uglify/minify scripts. But there have been talks of having an alternative way of pulling in scripts in prod (e.g. from a CDN). That's definitely a feature for the future though.

@filipesilva filipesilva self-assigned this Nov 2, 2016
@filipesilva filipesilva added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed P5 The team acknowledges the request but does not plan to address it, it remains open for discussion labels Nov 2, 2016
@hansl hansl modified the milestone: RC1 Nov 11, 2016
@hansl
Copy link
Contributor

hansl commented Feb 23, 2017

I think this is fixed by now. @filipesilva can you confirm? Closing this but should be reopened if there's still an issue.

@hansl hansl closed this as completed Feb 23, 2017
@filipesilva
Copy link
Contributor

Unfortunately this still happens. Workaround is to use minified scripts. But we should fix it if we can.

@filipesilva filipesilva reopened this Feb 23, 2017
@hansl hansl removed this from the RC1 milestone May 2, 2017
filipesilva added a commit to filipesilva/angular-cli that referenced this issue Aug 15, 2017
Adds sourcemap and minification to javascript added via the `scripts` array in `.angular-cli.json`.

`script-loader` is no longer used, which should help with CSP since it used `eval`.

Scripts will no longer appear in the console output for `ng build`, as they are now assets instead of webpack entry points.

It's no longer possible to have the `output` property of both a `scripts` and a `styles` entry pointing to the same file. This wasn't officially supported or listed in the docs, but used to be possible.

Fix angular#2796
Fix angular#7226
Fix angular#7290

Related to angular#6872
filipesilva added a commit to filipesilva/angular-cli that referenced this issue Aug 16, 2017
Adds sourcemap and minification to javascript added via the `scripts` array in `.angular-cli.json`.

`script-loader` is no longer used, which should help with CSP since it used `eval`.

Scripts will no longer appear in the console output for `ng build`, as they are now assets instead of webpack entry points.

It's no longer possible to have the `output` property of both a `scripts` and a `styles` entry pointing to the same file. This wasn't officially supported or listed in the docs, but used to be possible.

Fix angular#2796
Fix angular#7226
Fix angular#7290

Related to angular#6872
filipesilva added a commit to filipesilva/angular-cli that referenced this issue Aug 17, 2017
Adds sourcemap and minification to javascript added via the `scripts` array in `.angular-cli.json`.

`script-loader` is no longer used, which should help with CSP since it used `eval`.

Scripts will no longer appear in the console output for `ng build`, as they are now assets instead of webpack entry points.

It's no longer possible to have the `output` property of both a `scripts` and a `styles` entry pointing to the same file. This wasn't officially supported or listed in the docs, but used to be possible.

Fix angular#2796
Fix angular#7226
Fix angular#7290

Related to angular#6872
Brocco pushed a commit that referenced this issue Aug 17, 2017
Adds sourcemap and minification to javascript added via the `scripts` array in `.angular-cli.json`.

`script-loader` is no longer used, which should help with CSP since it used `eval`.

Scripts will no longer appear in the console output for `ng build`, as they are now assets instead of webpack entry points.

It's no longer possible to have the `output` property of both a `scripts` and a `styles` entry pointing to the same file. This wasn't officially supported or listed in the docs, but used to be possible.

Fix #2796
Fix #7226
Fix #7290

Related to #6872
dond2clouds pushed a commit to d2clouds/speedray-cli that referenced this issue Apr 23, 2018
Adds sourcemap and minification to javascript added via the `scripts` array in `.angular-cli.json`.

`script-loader` is no longer used, which should help with CSP since it used `eval`.

Scripts will no longer appear in the console output for `ng build`, as they are now assets instead of webpack entry points.

It's no longer possible to have the `output` property of both a `scripts` and a `styles` entry pointing to the same file. This wasn't officially supported or listed in the docs, but used to be possible.

Fix angular#2796
Fix angular#7226
Fix angular#7290

Related to angular#6872
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants