-
Notifications
You must be signed in to change notification settings - Fork 77
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
[i18n] [build] [styles] Update and fix of gulp build. #627
Conversation
bahiirwa
commented
Apr 4, 2023
•
edited
Loading
edited
- Fix: Update the gulp packages and migrate tasks runners to gulp 4.
- Fix: Use of calc() to fix deprecated SCSS division rules.
- Fix: Compilation of .pot file.
- Add: New rules to package.json for local style builds and potential merge/tag build automation.
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.
Hi @bahiirwa great effort 👏 . I have noticed one immediate issue which I have commented. Additionally please modify the feature branch to base it on develop
and kindly send a PR to develop
.
bc23f3f
to
89edbe2
Compare
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.
@bahiirwa Great job. We are getting there. Please see my comments for the new SCSS and gulp syntax. Thanks.
gulpfile.js
Outdated
|
||
// Create POT out of PHP files | ||
gulp.task('prepare-source', function () { | ||
function prepare_source() { | ||
gulp.src('**/*.php') |
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.
In new gulp we are supposed to return a promise or promise-like to properly signal a task completion. Please read documentation here https://gulpjs.com/docs/en/getting-started/async-completion
So, for all gulp task-related functions, you need to return the stream.
function foo() {
return gulp.src(...).pipe(...);
}
Then gulp will correctly hint to us about the time it took.
When there are no streams to return, you have various options. I find using the built-in Promise to be very powerful and flexible.
return new Promise((resolve, reject) => {
// doSomething is an async task, but it is implemented using a callback pattern.
doSomething(() => {
resolve();
});
});
You can also use the reject
to tell gulp the task has failed.
gulpfile.js
Outdated
|
||
// Push updated po resource to transifex. | ||
gulp.task('update-transifex', ['prepare-source'], function () { | ||
function update_transifex() { | ||
prepare_source(); |
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.
The prepare_source
is an async function. It will need some time to complete. Given that this (prepare_source
) async function is properly implemented, we need to wait for it, before pushing the resource, else there will always be race conditions. (An old version of freemius-en.po
will be pushed).
Since prepare_source
is a gulp task, we can use the gulp series
composition.
function prepare_source() {
return gulp.src(...)...
}
function push_transifex() {
return gulp.src(languagesFolder + 'freemius-en.po').pipe(transifex.pullResource());
}
const update_transifex = series(prepare_source, push_transifex);
Please pardon me for any interface/syntactical mistakes I might have made.
gulpfile.js
Outdated
|
||
// Download latest *.po translations. | ||
gulp.task('download-translations', ['update-transifex'], function () { | ||
async function download_translations() { | ||
update_transifex(); |
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.
@bahiirwa This and all other direct usages of async tasks need refactoring as above.
gulpfile.js
Outdated
// Compile css only in dev mode. | ||
function watch() { | ||
gulp.watch( './assets/scss/*.scss', style ); |
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.
@bahiirwa You can use the blob ./assets/scss/**/*.scss
instead of the two watch
.
gulpfile.js
Outdated
// Run postcss processing for styles. | ||
function style() { | ||
let plugins = [ |
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.
@bahiirwa Please use const
gulpfile.js
Outdated
exports.watch = watch; | ||
exports.build = build; | ||
exports.default = 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.
@bahiirwa I would add the style
task here too and optimize them, running tasks in parallel which can be run. Like (just an example)
parallel(
style,
series(prepare_source, update_transifex, ...)
);
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.
@bahiirwa Thinking about it again 🤔 I would rather not have transifex in watch
and build
, instead would have them in a separate npm run translation
command.
The reason is, for most day-to-day tasks, we just need the style watcher and maybe in the future a JS uglifier (which we can add when needed). So I propose to
- Have
npm run dev
andnpm run build
deal with only style for now. - Create a separate command for explicitly extracting translation. This will just create/update the
pot
file. - Create a separate command for explicitly updating existing
.po
with thepot
file. - Create a separate command for uploading/syncing translations with transifex.
- Create a separate command to do the above 3, in series.
Please feel free to modify the tasks/order as you see fit. I am yet to dig deep into exactly how the translations are being uploaded/handled. I will do that once we are done with the changes required in the new gulp tasks.
package.json
Outdated
@@ -9,20 +9,28 @@ | |||
"main": "gulpfile.js", | |||
"dependencies": {}, | |||
"scripts": { | |||
"dev": "gulp watch", |
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.
@bahiirwa Once we will be done with the PR review, and once this PR will be ready for merge, please create a CONTRIBUTING.md
file and document how to use the new build system, and what to do for transifex etc... I am sure your documentation expertise will make it easy for new contributors ;)
… of calc() is deprecated and will be removed in Dart Sass 2.0.0.
…ill be removed in Dart Sass 2.0.0.
0772674
to
ec3b820
Compare
Closing this in preference of #685 |