build: no gulp and use npm scripts #697
Conversation
|
Thanks for taking the time to modernize the scripts and setup. They've fallen a bit behind. |
|
@valscion Yeah, WIP on removing outdated and unnecessary deps too |
|
I've now merged the prettier PR so all the other PRs you've worked on have conflicts now. Feel free to ping me (or convert the PRs first to draft and then to ready-to-review and ping me) once you want me to take a look at these. |
alexander-akait
force-pushed
the
no-gulp
branch
from
c2ce96c to
5ea7d73
Compare
|
@valscion Ready for merge |
valscion
left a comment
valscion
left a comment
There was a problem hiding this comment.
Looks good — We can merge this and continue cleanup or then do the cleanup in this PR and merge. Either way works for me.
| gulp.task("clean", gulp.parallel(cleanNodeScripts, cleanViewerScripts)); | ||
| gulp.task( | ||
| "build", | ||
| gulp.series("clean", compileNodeScripts, compileViewerScripts), | ||
| ); | ||
| gulp.task("watch", gulp.series("build", watch)); |
There was a problem hiding this comment.
Watching used to clean first before building — doesn't look like the same is happening now. Might not be a problem, though.
There was a problem hiding this comment.
We do the same build:analyzer (and other) run clean, watch:analyzer just pass --watch, so we clean these things like we have before
|
|
||
| const isDev = opts.env === "dev"; | ||
| const isDev = (process.env.NODE_ENV || "production") === "development"; | ||
| const needAnalyze = process.env.ANALYZE || false; |
There was a problem hiding this comment.
Is this process.env.ANALYZE used somewhere? It doesn't seem like it has been used in the past and I didn't spot any code changes here which would put it to use. So is this value always false now?
We can get rid of the if (needAnalyze) block altogether if that serves no real purpose anymore.
I don't think I ever used the -a flag which used to be present in the gulpfile.js myself...
There was a problem hiding this comment.
I put this in future to analyze own viewer code, may be useful
|
@valscion let's merge this and continue in other PRs for better history |
Summary
We don't need gulp and extra outdated deps to make this simple things
What kind of change does this PR introduce?
build
Did you add tests for your changes?
Existing
Does this PR introduce a breaking change?
No
If relevant, what needs to be documented once your changes are merged or what have you already documented?
Nothing