-
Notifications
You must be signed in to change notification settings - Fork 11
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
v7.9.0 (CPD-11491) - Optional removal of console.log statements #112
Changes from all commits
608ccda
8fc9baf
a7c0a90
cf4c509
4b1933a
429d924
144650e
4e3f432
87e65e9
49aa7e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ashleynolan Should I add that in this PR? Fixed in latest commit. |
||
stripDebug() | ||
)) | ||
|
||
|
There was a problem hiding this comment.
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,