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

gulp fixes and reverts #542

Merged
merged 4 commits into from
Sep 14, 2015
Merged

Conversation

nmccready
Copy link
Contributor

So bottom line I am not sure why everything was dandy Friday. Anyways things are back to working order. The build times are not as glorious as Friday, but that could be due to more admin files.

@@ -51,12 +51,6 @@ app.use helmet.nocache()
# ensure all assets and data are compressed - above static
app.use compress()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Killed dep as @zacronos mentioned.. this was not needed

@nmccready
Copy link
Contributor Author

@zacronos @realtyjustin @jessecstewart looking for 2 LGTMS

@@ -13,13 +13,5 @@ gulp.task 'watch_all_front', gulp.parallel 'angularWatch', 'angularWatchAdmin'
gulp.task 'watch', gulp.series 'watch_all_front', (done) ->
gulp.watch ['gulp/**/*.coffee','spec/gulp/**/*.coffee', specCommon], gulp.series 'gulpSpec'
gulp.watch ['backend/**/*.coffee', 'spec/backend/**/*.coffee', specCommon], gulp.series 'backendSpec'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

paths.destFull.scripts + '/map.bundle.js',      
-    paths.destFull.scripts + '/map.templates.js',      
-    paths.destFull.scripts + '/admin.bundle.js',       
-    paths.destFull.scripts + '/admin.templates.js',

because this is an artifact and it should not be watched. This was causing extra build times and and re-kicking the build as files were being moved and shuffled by gulp. We don't need to watch the artifacts we need to watch the frontend stuff which we are already doing via watch_all_front .

Eliminating this also eliminates the need to have watch run at a specific time. Heck it could even be added to be prior to clean via parallel.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was my bad, I listed the artifacts so when they were rebuilt, Karma would run specs. But looking at the Karma docs it looks like it watches everything configured within files by default (http://karma-runner.github.io/0.8/config/files.html). That does make me wonder why Karma wasn't triggering twice each time though.. am I understanding this correctly @nmccready ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The watch inside karma itself is off so all watching is done by gulp. This is done to avoid complications.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, will specs still run on a rebuild?

@zacronos
Copy link
Member

LGTM


gulp.task 'developNoSpec', gulp.series 'clean', 'otherAssets', gulp.parallel('express', 'watch')
#this allows `gulp help` task to work which will display all taks via CLI so yes it is used
Copy link
Contributor Author

Choose a reason for hiding this comment

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

notes on gulp-help
chmontgomery/gulp-help#31

@realtyjustin
Copy link
Contributor

Confirmed the admin.css and admin.templates.js are being build as expected, as well as watches on common and admin seem to rebuild upon file update as expected as well, too - thx for fixing those
LGTM

nmccready added a commit that referenced this pull request Sep 14, 2015
@nmccready nmccready merged commit 31a6a04 into realtymaps:master Sep 14, 2015
@zacronos zacronos deleted the dev/nem/gulpFixes branch September 30, 2015 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants