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

[i18n] [build] [styles] Update and fix of gulp build. #627

Closed
wants to merge 17 commits into from

Conversation

bahiirwa
Copy link
Contributor

@bahiirwa bahiirwa commented Apr 4, 2023

  • 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.

@bahiirwa bahiirwa changed the title Feature/update gulp dependencies Update & Fix gulp build Apr 4, 2023
@bahiirwa bahiirwa changed the title Update & Fix gulp build [i18n] [build] [styles] Update and fix of gulp build. Apr 4, 2023
Copy link
Contributor

@swashata swashata left a 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.

gulpfile.js Outdated Show resolved Hide resolved
@bahiirwa bahiirwa closed this Apr 7, 2023
@bahiirwa bahiirwa force-pushed the feature/update-gulp-dependencies branch from bc23f3f to 89edbe2 Compare April 7, 2023 07:55
@bahiirwa bahiirwa reopened this Apr 9, 2023
@bahiirwa bahiirwa changed the base branch from master to develop April 12, 2023 05:10
Copy link
Contributor

@swashata swashata left a 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.

assets/css/admin/account.css Outdated Show resolved Hide resolved
assets/scss/admin/_switch.scss Outdated Show resolved Hide resolved
gulpfile.js Outdated

// Create POT out of PHP files
gulp.task('prepare-source', function () {
function prepare_source() {
gulp.src('**/*.php')
Copy link
Contributor

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();
Copy link
Contributor

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();
Copy link
Contributor

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 );
Copy link
Contributor

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 = [
Copy link
Contributor

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(
Copy link
Contributor

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, ...)
);

Copy link
Contributor

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

  1. Have npm run dev and npm run build deal with only style for now.
  2. Create a separate command for explicitly extracting translation. This will just create/update the pot file.
  3. Create a separate command for explicitly updating existing .po with the pot file.
  4. Create a separate command for uploading/syncing translations with transifex.
  5. 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",
Copy link
Contributor

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 ;)

@bahiirwa bahiirwa force-pushed the feature/update-gulp-dependencies branch from 0772674 to ec3b820 Compare July 18, 2023 07:07
@bahiirwa
Copy link
Contributor Author

Closing this in preference of #685

@bahiirwa bahiirwa closed this Jan 31, 2024
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.

2 participants