-
Notifications
You must be signed in to change notification settings - Fork 593
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
JavaScript build cleanup & simplifications #2310
Conversation
js/gulpfile.js
Outdated
deleteAsync(libGeneratedFiles(lib, sources)); | ||
cb(); | ||
}); | ||
const slicePatterns = [`../slice/${lib}/*.ice`, ...(excludes[lib] || [])]; |
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.
Could we move this const
up to the top, since it looks like we repeat this string in the above block.
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.
Yes, good catch.
js/gulpfile.js
Outdated
pump( | ||
[ | ||
gulp.src(sources.slice.map(sliceFile)), | ||
gulp.src([`../slice/${lib}/*.ice`, ...(excludes[lib] || [])]), |
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.
Here ^
|
||
gulp.task( | ||
"test:clean", | ||
gulp.series( |
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.
Very minor, but why switch the cleaning from parallel to series?
More consistent with the other tasks we define I guess?
I'm not against it, just curious.
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.
We got a few out of memory errors in GitHub Action workers
This PR cleanups and simplifies the JavaScript build