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.

Sign up for GitHub

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

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

Merged
merged 10 commits into from
Feb 5, 2018
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
608ccda
CPD-11491: Make removal of console statements for prod builds optional
xander-marjoram Feb 1, 2018
8fc9baf
Merge branch 'master' into CPD-11491
xander-marjoram Feb 1, 2018
a7c0a90
CPD-11491: Remember that you can use actual logical operators. Roll b…
xander-marjoram Feb 2, 2018
cf4c509
CPD-11491: Formalities
xander-marjoram Feb 2, 2018
4b1933a
CPD-11491: Merge fixes
xander-marjoram Feb 2, 2018
429d924
CPD-11491: Updated readme
xander-marjoram Feb 2, 2018
144650e
CPD-11491: Minor config updates
xander-marjoram Feb 5, 2018
4e3f432
CPD-11491: Config unit test updates
xander-marjoram Feb 5, 2018
87e65e9
CPD-11491: Add console flag
xander-marjoram Feb 5, 2018
49aa7e4
CPD-11491: Make test descriptions more accurately reflect the names i…
xander-marjoram Feb 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,16 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).


v7.9.0
------------------------------
*February 2, 2018*

### Added
- `stripDebug` option to `config.js` to allow the inclusion of console statements in production builds.

### Changed
- Jest rollback from v22.1.4 to v21.2.1.

v7.8.0
------------------------------
*February 1, 2018*
Expand All @@ -16,7 +26,6 @@ v7.8.0
### Changed
- Minor package updates


v7.7.0
------------------------------
*January 30, 2018*
Expand Down
13 changes: 10 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,8 @@ Here is the outline of the configuration options, descriptions of each are below
],
jsDir,
lintPaths,
usePackageVersion
usePackageVersion,
stripDebug
},
img: {
imgDir,
Expand Down Expand Up @@ -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'`.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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



### `js`
Expand Down Expand Up @@ -506,7 +507,13 @@ 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 JS file e.g `'filename-[version].js'`.
Default is set to `false`, when set to true this will bundle a versioned JS file e.g `'filename-[version].js'`.

- #### `stripDebug`

Type: `boolean`
Copy link
Contributor

@DamianMullins DamianMullins Feb 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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


Default is set to `true`. When set to false, this will prevent `console.log()` statements from being removed for production builds. **For non-production builds, this has no effect: you will still get debug statements even if you've set this to `true`.**


### `img`
Expand Down
3 changes: 2 additions & 1 deletion config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ const ConfigOptions = () => {
},
jsDir: 'js',
lintPaths: [''],
usePackageVersion: false
usePackageVersion: false,
stripDebug: true
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in latest commit 👍

},

/**
Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@justeat/gulp-build-fozzie",
"version": "7.8.0",
"version": "7.9.0",
"description": "Gulp build tasks for use across Fozzie modules",
"main": "index.js",
"author": "Damian Mullins <damian.mullins@just-eat.com> (http://www.damianmullins.com)",
Expand Down Expand Up @@ -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",
Copy link
Contributor

@DamianMullins DamianMullins Feb 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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,

"merge-stream": "^1.0.1",
"postcss-assets": "^5.0.0",
"postcss-reporter": "^5.0.0",
Expand Down
2 changes: 1 addition & 1 deletion tasks/javascript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@xander-marjoram xander-marjoram Feb 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Fixed in latest commit.

stripDebug()
))

Expand Down
Loading