feat(index): add `options` validation (`schema-utils`) by hulkish · Pull Request #80 · webpack-contrib/uglifyjs-webpack-plugin

hulkish · 2017-07-12T07:11:53Z

Closes #4, #64.

alexander-akait · 2017-07-12T09:42:33Z

+    const { uglifyOptions } = this.options;
+    const uglifiedAssets = new WeakSet();
+
+    if (typeof this.options.sourceMap === 'undefined' && (compiler.options.devtool === 'sourcemap' || compiler.options.devtool === 'source-map')) {


Seems not related to this PR

You're right. I can separate this into a diff pr if u like. However, I think it's been a missing piece to this plugin for some time now. It's also related to the options so i figured it would be good to include.

Not to mention - this same logic is built into webpack core. I believe this responsibility belongs in this plugin.

If this pr is merged i plan to follow up by making a pr at webpack core repo to extract this logic from there. This also doesn't introduce a breaking change and all tests are passing.

@hulkish let's wait what other will say, but be good have separate PR for this, it is allow to maintenance CHANGELOG

If this pr is merged i plan to follow up by making a pr at webpack core repo to extract this logic from there. This also doesn't introduce a breaking change and all tests are passing.

btw good enchantment!

bahaha : 🌮

hulkish · 2017-07-12T10:48:44Z

-const defaultUglifyOptions = {
-  output: {
-    comments: /^\**!|@preserve|@license|@cc_on/,
-    beautify: false,


Removed these defaults because these are the same defaults uglify-es uses internally:

beautify: false, semicolons: true, shebang: true,

hulkish · 2017-07-12T10:52:37Z

  ],
  "scripts": {
    "start": "npm run build -- -w",
-    "build": "cross-env NODE_ENV=production babel src -d dist --ignore 'src/**/*.test.js'",


added this for babel-cli to auto-copy src/options-schema.json to the dist folder. (to prevent MODULE_NOT_FOUND error on published code)

=> webpack-defaults, but options.json in the root is also ok imho, since it is metadata, still needs to be added to pkg.files then, so really no preference :)

hulkish · 2017-07-12T10:56:15Z

-    this.uglifyOptions = this.options.uglifyOptions || {};
-  }
-
-  static buildDefaultUglifyOptions({ ecma, warnings, parse = {}, compress = {}, mangle, output, toplevel, ie8 }) {


Simplified this by leveraging destructering with default values in the plugin's constructor.

hulkish · 2017-07-12T11:06:50Z

@@ -0,0 +1,50 @@
+{


Reworked from #64

hulkish · 2017-07-12T11:33:46Z

-    const uglifyOptions = UglifyJsPlugin.buildDefaultUglifyOptions(this.uglifyOptions);
-    // Making sure output options exists if there is an extractComments options
-    if (this.options.extractComments) {
-      uglifyOptions.output = uglifyOptions.output || {};


replaced with logic in constructor

michael-ciniawsky

Please try to separate changes into logical pieces (PR's) whenever possible. The weakSet hoisting and options refactoring is strictly speaking a separate PR :) refactor(index): ....

michael-ciniawsky · 2017-07-12T22:01:21Z

 import ModuleFilenameHelpers from 'webpack/lib/ModuleFilenameHelpers';
+import validateOptions from 'schema-utils';
 import uglify from 'uglify-es';
+import optionsSchema from './options-schema.json';


optionsSchema => schema

michael-ciniawsky · 2017-07-12T22:02:30Z

-      this.options = options || {};
-    }
+  constructor(options = {}) {
+    validateOptions(optionsSchema, options, 'UglifyJsPlugin');


UglifyJsPlugin => UglifyJS Plugin

michael-ciniawsky · 2017-07-12T22:06:21Z

@@ -0,0 +1,50 @@
+{


src/options-schema.json => ./options.json, if it should reside in the src and needs to be copied, the change needs to be made in webpack-defaults beforehand as always 😛

michael-ciniawsky · 2017-07-12T22:07:23Z

+          }
+        },
+        "output": {
+          "type": ["object", "null"]


Is null valid ? 🙃

yes, it's default value is null: https://github.com/mishoo/UglifyJS2#user-content-minify-options

I was referring to the schema validation here :) e.g {Regex}, {Function} aren't supported by ajv (schema-utils) yet.

michael-ciniawsky · 2017-07-12T22:08:52Z

+      "properties": {
+        "ecma": {
+          "type": "number",
+          "enum": [5, 6, 7, 8]


Could you make this throw and see if the ValidationError is still displayed correctly with this additional info please?

michael-ciniawsky · 2017-07-12T22:18:25Z

  ],
  "scripts": {
    "start": "npm run build -- -w",
-    "build": "cross-env NODE_ENV=production babel src -d dist --ignore 'src/**/*.test.js'",


=> webpack-defaults, but options.json in the root is also ok imho, since it is metadata, still needs to be added to pkg.files then, so really no preference :)

hulkish · 2017-07-13T17:26:52Z

ok done @michael-ciniawsky 2a3687e

michael-ciniawsky · 2017-07-20T23:39:12Z

+    "uglifyOptions": {
+      "additionalProperties": true,
+      "type": "object",
+      "properties": {


michael-ciniawsky · 2017-07-20T23:39:56Z

@@ -0,0 +1,51 @@
+{
+  "type": "object",
+  "properties": {


include, exclude, parallel ?

michael-ciniawsky · 2017-07-20T23:41:26Z

  "files": [
-    "dist"
+    "dist",
+    "options.json"


I'm 💯 sry about this, your initial way of handling this file via --copy-files in the build script is the better solution, please revert :) 👍

michael-ciniawsky · 2017-07-21T01:05:40Z

 import RequestShortener from 'webpack/lib/RequestShortener';
 import ModuleFilenameHelpers from 'webpack/lib/ModuleFilenameHelpers';
+import validateOptions from 'schema-utils';
+import schema from '../options.json';


../options.json => ./options.json (../ => ./) ?

hulkish added semver: Minor type: Feature labels Jul 12, 2017

hulkish requested review from bebraw, joshwiens and michael-ciniawsky July 12, 2017 07:20

alexander-akait reviewed Jul 12, 2017

View reviewed changes

hulkish self-assigned this Jul 12, 2017

hulkish commented Jul 12, 2017

View reviewed changes

Comment thread src/options-schema.json

@@ -0,0 +1,50 @@

{

hulkish Jul 12, 2017

Copy link
Copy Markdown

Contributor Author

Choose a reason for hiding this comment

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

Reworked from #64

hulkish changed the title ~~feat: add options validation~~ feat(index): add options validation Jul 12, 2017

hulkish commented Jul 12, 2017

View reviewed changes

hulkish mentioned this pull request Jul 12, 2017

feat: support devtool (options.sourceMap) #81

Closed

hulkish force-pushed the hulkish-OptimizeAndValidateOptions branch from c8673e1 to 0810394 Compare July 12, 2017 12:27

michael-ciniawsky suggested changes Jul 12, 2017

View reviewed changes

michael-ciniawsky mentioned this pull request Jul 12, 2017

feat(index): add options validation #64

Closed

michael-ciniawsky added priority: 3 (required) pr_action: Review labels Jul 12, 2017

michael-ciniawsky added this to the 1.0.0 milestone Jul 12, 2017

michael-ciniawsky added the status: Pending label Jul 12, 2017

michael-ciniawsky changed the title ~~feat(index): add options validation~~ feat(index): add options validation Jul 12, 2017

michael-ciniawsky approved these changes Jul 13, 2017

View reviewed changes

This was referenced Jul 19, 2017

fix(index): deep cloning options (options.uglifyOptions) #87

Closed

feat: add support for parallelization && caching (options.parallel) #77

Merged

michael-ciniawsky added pr_action: Rebase and removed pr_action: Review labels Jul 20, 2017

michael-ciniawsky changed the title ~~feat(index): add options validation~~ feat(index): add options validation (schema-utils) Jul 20, 2017

michael-ciniawsky reviewed Jul 20, 2017

View reviewed changes

hulkish added 6 commits July 20, 2017 19:54

added schema-utils validateOptions, created options schema

0162c89

added --copy-files arg to babel for build script for options-schema.json

782ae67

added tests for thrown ValidationError's

6bba12d

extracted auto-sourceMap feature into separate pr

5e1ed5c

corrected uglifyOptions.ecma schema, added tests for ecma option

21a6d64

applied requested changes

62211e3

hulkish force-pushed the hulkish-OptimizeAndValidateOptions branch from 2a3687e to 38f2cac Compare July 20, 2017 23:56

added another test for ecma option

5d1ffea

hulkish force-pushed the hulkish-OptimizeAndValidateOptions branch from 38f2cac to 5d1ffea Compare July 20, 2017 23:58

hulkish added 2 commits July 20, 2017 20:21

added schema definition for parallel option

c956270

removed defaults for parallel option schema definition

49dcfef

michael-ciniawsky added pr_action: Review and removed pr_action: Rebase labels Jul 21, 2017

hulkish added 2 commits July 20, 2017 20:56

added more options schems definitions

e01422c

restored babel --copy-files strategy for options.json

ea3e3c2

michael-ciniawsky reviewed Jul 21, 2017

View reviewed changes

updated options.json path

441653a

michael-ciniawsky approved these changes Jul 21, 2017

View reviewed changes

michael-ciniawsky added status: Approved and removed priority: 3 (required) pr_action: Review status: Pending labels Jul 21, 2017

michael-ciniawsky merged commit f19b2de into master Jul 21, 2017

michael-ciniawsky deleted the hulkish-OptimizeAndValidateOptions branch July 21, 2017 04:04

michael-ciniawsky removed this from the 2.0.0 milestone Oct 29, 2017

snyk-bot mentioned this pull request Jun 2, 2020

[Snyk] Security upgrade serialize-javascript from 1.5.0 to 3.1.0 A-Mikhail/uglifyjs-webpack-plugin#2

Open

Uh oh!

Uh oh!

Conversation

hulkish commented Jul 12, 2017 • edited by michael-ciniawsky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hulkish Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hulkish Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hulkish Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hulkish Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hulkish commented Jul 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hulkish commented Jul 12, 2017 •

edited by michael-ciniawsky

Loading

hulkish Jul 12, 2017 •

edited

Loading

hulkish Jul 12, 2017 •

edited

Loading

hulkish Jul 12, 2017 •

edited

Loading

hulkish Jul 12, 2017 •

edited

Loading

michael-ciniawsky Jul 12, 2017 •

edited

Loading