v7.9.0 (CPD-11491) - Optional removal of console.log statements #112

xander-marjoram · 2018-02-02T14:58:04Z

Added a value to the config.js that can override the removal of console.log() statements that would usually happen automatically for production builds.

Had to revert Jest to 21.2.1 as it was causing problems.

…ack jest version from 22.1.4 to 21.2.1.

ashleynolan · 2018-02-02T15:22:20Z

Few conflicts need resolving

fozzie-bot · 2018-02-02T15:24:51Z

	Messages
📖	🔥 👏 You’re a deletion machine!

Generated by 🚫 dangerJS

xander-marjoram · 2018-02-02T15:25:47Z

@ashleynolan Resolved

DamianMullins · 2018-02-02T15:52:51Z

README.md

+
+- #### `stripDebug`
+
+  Type: `boolean`


Should add the default type like some of the other options do above.

DamianMullins · 2018-02-02T15:53:28Z

README.md

@@ -443,7 +444,7 @@ Will add a content hash to the JS and CSS filenames, generating a new filename i

  Type: `boolean`

-  Default is set to false, when set to true this will bundle a versioned css file e.g `'filename-[version].css'`.
+  Default is set to `false`, when set to true this will bundle a versioned css file e.g `'filename-[version].css'`.


Looks like this one is also missing the default option..

That's my bad, I can fix this in my PR as part of the original ticket unless @xander-marjoram wants to amend it here? :)

@kevinrodrigues I'll fix it here, if you haven't already 👍

DamianMullins · 2018-02-02T15:55:41Z

config.js

@@ -45,7 +45,8 @@ const ConfigOptions = () => {
            },
            jsDir: 'js',
            lintPaths: [''],
-            usePackageVersion: false
+            usePackageVersion: false,
+            stripDebug: true


Can you add the relevant unit tests to config.test.js please?

Done in latest commit 👍

kevinrodrigues · 2018-02-02T16:02:47Z

tasks/javascript.js

@@ -134,7 +134,7 @@ gulp.task('scripts:bundle', () => {
            ))

            // if production build, rip out our console logs
-            .pipe(gulpif(config.isProduction,
+            .pipe(gulpif(config.isProduction && config.js.stripDebug,


Might be a silly question but why do we need to have this new option? Shouldn't we strip console logs / debuggers from production code anyway? Or is there a reason we use them in production.

It's a request from the OrderWeb team. They want to have production type code with debug statements for when they release to QA.

One thing that would be worth adding is the ability to set this config variable through an environment variable. So something like gulp --stripDebug=false would set this to false (as I imagine that might be how they need to set it to be different on every release server).

@ashleynolan Should I add that in this PR?
(Verbal answer was "yes")

Fixed in latest commit.

ashleynolan

Looks good once those comments from D-dogg have been committed 👍

DamianMullins · 2018-02-02T16:20:00Z

package.json

@@ -71,7 +71,7 @@
    "handlebars-helpers": "^0.10.0",
    "helper-markdown": "^1.0.0",
    "helper-md": "^0.2.2",
-    "jest-cli": "^22.1.4",
+    "jest-cli": "^21.2.1",


For future reference, one of the issues with v22.1.4 was that an error of "TypeError: environment.setup is not a function" was being thrown in some repos, there is a discussion on the Jest GitHub repo where it seems like one of the dependencies hasn't yet been updated,

kevinrodrigues · 2018-02-05T10:10:38Z

test/config.test.js

+        expect(config.js.stripDebug).toBe(true);
+    });
+
+    it('strip debug can be updated', () => {


stripDebug.

@kevinrodrigues Should I change the ones that say 'is production' as well then, and things like 'scss directory' rather than 'scssDir'?

I think so. There's a mixture of descriptions in this file, usually I add the 'thing' I'm talking about or testing as "methodName" `` (with backticks) so it's clear what we're actually referring too in the test itself, I've also found it helps to find that reference quicker when a test fails. :)

Let's keep things consistent with these names? All of the existing tests use full, space separated words. I don't mind which one we go with as long as they are consistent 🇸🇱

It's probably one to come back to at some point though.

I've chosen to use the variable names as they are found in the config, but with backticks to make it clearer.

DamianMullins · 2018-02-05T12:05:46Z

test/config.test.js

+        expect(config.js.usePackageVersion).toBe(false);
+    });
+
+    it('usePackageVersion can be updated', () => {


No backticks around usePackageVersion here.

…n the config object

xander-marjoram added 4 commits February 1, 2018 09:25

CPD-11491: Make removal of console statements for prod builds optional

608ccda

Merge branch 'master' into CPD-11491

8fc9baf

CPD-11491: Remember that you can use actual logical operators. Roll b…

a7c0a90

…ack jest version from 22.1.4 to 21.2.1.

CPD-11491: Formalities

cf4c509

xander-marjoram requested a review from a team as a code owner February 2, 2018 14:58

CPD-11491: Merge fixes

4b1933a

xander-marjoram changed the title ~~v7.8.0 (CPD-11491) - Optional removal of console.log statements~~ v7.9.0 (CPD-11491) - Optional removal of console.log statements Feb 2, 2018

CPD-11491: Updated readme

429d924

xander-marjoram force-pushed the CPD-11491 branch from 5a8e090 to 429d924 Compare February 2, 2018 15:38

DamianMullins reviewed Feb 2, 2018

View reviewed changes

kevinrodrigues reviewed Feb 2, 2018

View reviewed changes

ashleynolan reviewed Feb 2, 2018

View reviewed changes

DamianMullins reviewed Feb 2, 2018

View reviewed changes

xander-marjoram added 2 commits February 5, 2018 09:35

CPD-11491: Minor config updates

144650e

CPD-11491: Config unit test updates

4e3f432

kevinrodrigues reviewed Feb 5, 2018

View reviewed changes

DamianMullins reviewed Feb 5, 2018

View reviewed changes

CPD-11491: Add console flag

87e65e9

xander-marjoram force-pushed the CPD-11491 branch from f681170 to 87e65e9 Compare February 5, 2018 13:17

CPD-11491: Make test descriptions more accurately reflect the names i…

49aa7e4

…n the config object

xander-marjoram force-pushed the CPD-11491 branch from feb83d5 to 49aa7e4 Compare February 5, 2018 13:42

DamianMullins approved these changes Feb 5, 2018

View reviewed changes

kevinrodrigues approved these changes Feb 5, 2018

View reviewed changes

kevinrodrigues merged commit dd7984a into master Feb 5, 2018

kevinrodrigues deleted the CPD-11491 branch February 5, 2018 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v7.9.0 (CPD-11491) - Optional removal of console.log statements #112

v7.9.0 (CPD-11491) - Optional removal of console.log statements #112

xander-marjoram commented Feb 2, 2018

ashleynolan commented Feb 2, 2018

fozzie-bot commented Feb 2, 2018 •

edited

Loading

xander-marjoram commented Feb 2, 2018

DamianMullins Feb 2, 2018 •

edited

Loading

DamianMullins Feb 2, 2018

kevinrodrigues Feb 2, 2018

xander-marjoram Feb 5, 2018

DamianMullins Feb 2, 2018

xander-marjoram Feb 5, 2018

kevinrodrigues Feb 2, 2018

ashleynolan Feb 2, 2018

ashleynolan Feb 2, 2018

xander-marjoram Feb 5, 2018 •

edited

Loading

ashleynolan left a comment

DamianMullins Feb 2, 2018 •

edited

Loading

kevinrodrigues Feb 5, 2018

xander-marjoram Feb 5, 2018 •

edited

Loading

kevinrodrigues Feb 5, 2018

DamianMullins Feb 5, 2018 •

edited

Loading

xander-marjoram Feb 5, 2018

DamianMullins Feb 5, 2018

v7.9.0 (CPD-11491) - Optional removal of console.log statements #112

v7.9.0 (CPD-11491) - Optional removal of console.log statements #112

Conversation

xander-marjoram commented Feb 2, 2018

ashleynolan commented Feb 2, 2018

fozzie-bot commented Feb 2, 2018 • edited Loading

xander-marjoram commented Feb 2, 2018

DamianMullins Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xander-marjoram Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

ashleynolan left a comment

Choose a reason for hiding this comment

DamianMullins Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xander-marjoram Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DamianMullins Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fozzie-bot commented Feb 2, 2018 •

edited

Loading

DamianMullins Feb 2, 2018 •

edited

Loading

xander-marjoram Feb 5, 2018 •

edited

Loading

DamianMullins Feb 2, 2018 •

edited

Loading

xander-marjoram Feb 5, 2018 •

edited

Loading

DamianMullins Feb 5, 2018 •

edited

Loading