-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Major gulp refactor: blueprint.defineTaskGroup() #617
Conversation
defines a task group based on a block query with some cool options. gulp task itself is defined in callback so it can be configured precisely as needed. this removes the need to specify deps in config. function accepts options object to support more complex usage and multiple args.
for tasks that care about project dependencies 😄
sass-compile => sass sass-lint => stylelint typescript-compile => tsc typescript-lint => tslint webpack-compile-docs => webpack-docs webpack-compile-w-docs => webpack-docs-watch -w- => :only for incremental build tasks so: sass-compile-table => sass-table:only dependencies are much easier to trace now because they're all listed in task definition.
* The task name is of the format `[name]-[project.id]`. | ||
* Finally, a "group task" is defined with the base name that runs all the project tasks. | ||
* This group task can be configured to run in parallel or in sequence. | ||
* @param {{block: string, name?: string, parallel?: boolean}} options |
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.
instead of "block", I propose another name: "scope"
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.
love it!
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.
actually i'm less into scope
now. config block
sounds better than config scope
, as does projectsWithBlock
vs projectsWithScope
.
i am open to a new name though, just don't think we've found it yet.
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.
block really doesn't mean anything. what do you mean by "config block sounds better than config scope"? also projectsWithScope
sounds fine to me
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.
what is a scope?
gulp.src([ | ||
path.join(project.cwd, "!(dist|node_modules|typings)", "**", "*.{js,jsx,ts,tsx}"), | ||
// exclude nested dist directories (ex: table/preview/dist) | ||
"!" + path.join(project.cwd, "*", "dist", "**", "*.{js,jsx,ts,tsx}"), |
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.
this looks pretty awkward, is it a new consideration we have to take with this build system refactor?
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.
no, it's always been there. it's actually due to table's webpack-based build which doesn't rely on these changes even a little bit.
overally looks good, just minor comments ^ |
@giladgray FYI I'm reading through all the existing Gulp code now, then I'll circle back to read through this afterward. |
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.
Phew, what a marathon. Finally perused all the changes, @giladgray. Left a few clarifying questions, but otherwise this looks good.
gulp/isotest.js
Outdated
parallel: false, | ||
}, (taskName, project) => { | ||
gulp.task(taskName, [`typescript-compile-${project.id}`], () => { | ||
return gulp.src(project.cwd + "test.iso/**/*") |
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.
@giladgray just gonna comment on every little thing until I build up a more solid understanding of this whole build system. There's a minor change in behavior here, yes (${project.cwd}/test/isotest.js
=> ${project.cwd}/test.io/**/*
)? Is the reasoning that we expect to add more isomorphic test files later, so might as well support anything in that directory moving forward?
// allow for no-op on project dependencies | ||
if (project.copy === false) { | ||
return; | ||
} |
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.
So this check is gone now. In the new code, seems like omitting a key/blockname (e.g. copy
) from a project config would cause projectsWithBlock
to return an empty array in blueprint.taskGroup, which would then define a gulp.task
that simply executes an empty set of sub-tasks—effectively a no-op. Is that behavior drastically different from this special no-op case above that used to be here? Was it ever particularly helpful or necessary?
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.
blueprint.task
always generated a root task that depended on corresponding tasks in other projects. so copy-files-docs
depended on (the existence of) copy-files-core
, which is why we had to add the no-op shortcut. this was awkward because there wasn't actually a dependency between the copy tasks (like, they're completely separate operations), whereas there is a real dependency for the typescript tasks (core must be compiled before datetime).
in this new world, the copy
task simply ignores the depTaskNames
argument and voila, packages can be copied without any dependencies, as they should be!
gulp/index.js
Outdated
const taskName = [name, project.id].join("-"); | ||
taskFn(taskName, project); | ||
const taskName = [name, id].join("-"); | ||
const depNames = dependencies.map((dep) => [name, dep].join("-")); |
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.
It appears that dep
and id
both project IDs in practice (e.g. "core", "datetime", "table"). It'd be helpful to name dep
to depId
IMO, just so it's clear that the names we're building on this line and the line above both follow the same scheme.
webpack(configuration, webpackConfig.webpackDone(callback)); | ||
}); | ||
|
||
gulp.task("webpack-compile-w-docs", (callback) => { // eslint-disable-line no-unused-vars |
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.
Do we not need the eslint-disable
line anymore? Looks like we're still not using the callback
argument anywhere. Did we need that comment in the first place?
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 are not using eslint anymore! it's all TSLint
@@ -17,8 +17,8 @@ module.exports = (blueprint, gulp, plugins) => { | |||
return del(cleanDirs, { force: true }); | |||
}); | |||
|
|||
gulp.task("tslint", () => ( | |||
gulp.src(["*.js", "gulp/**/*.js", "packages/*/*.js"]) | |||
gulp.task("tslint-gulp", () => ( |
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.
What's the motivation behind this -gulp
-suffixed tslint
task as distinct from the non-suffixed tslint
task? Looks like we're tslint
-ing all JS files...and then also JS files within gulp/
? Is that necessary on non-typescript files?
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.
ok so: the previous iteration of this task ran tslint on all .js
files (hence the package/**/*.js
glob).
this new iteration runs tslint on the gulp code only because JS files in packages are now linted by tslint-<package>
.
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.
Gotcha 👍
gulp/typescript.js
Outdated
// create a TypeScript project for each project to improve re-compile speed. | ||
// this must be done outside of a task function so it can be reused across runs. | ||
const tsconfigPath = path.join(project.cwd, "tsconfig.json"); | ||
project.typescriptProject = typescript.createProject(tsconfigPath, { |
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.
Where does typescript
come from? Doesn't appear to be defined in this file.
What is a task group?
A task group is a set of gulp tasks that performs the same operation on every relevant package, and a parent task that runs each of the package tasks respecting dependencies between packages. Relevant packages are those that have a given "block" present in their definition in Gulpfile.js.
For example, the "sass" group compiles .scss files to css.
gulp sass
will runsass-core
,sass-datetime
,sass-table
, andsass-docs
.sass-core
will be run first, as the other three depend on it, thensass-datetime
andsass-table
in parallel, followed bysass-docs
which depends on all of them.How was it before?
The previous iteration of our gulp build (before this PR) used
blueprint.task(block, name, deps, fn)
to define a task group. This function was expressive but did not offer control of the task function itself. Instead, thefn
arg received(package, isDevMode)
and returned a stream or a promise. It was not able to use the simplerdone
callback syntax forgulp.task()
, so a handful of our task groups were defined usingblueprint.projectsWithBlock()
directly, leading to some unsightly code.Changes proposed in this pull request:
blueprint.defineTaskGroup(options, fn)
defines a task group based on a block query with some cool options. The gulp task itself is defined in the callback so it can be configured precisely as needed.This is a big improvement over the previous iteration because it provides full control over the task config: you can use
gulp.task()
exactly as you need (deps, no deps,done
arg, etc). It also means that the internals ofdefineTaskGroup
are straightforward and transparent—there's very little magic here (comparetask
todTG
).Many of the compile tasks have been renamed. They are now shorter and more accurately describe what they do.
copy: false
in config)The
-w-
segment for incremental tasks run by watchers has been renamed to:only
suffix. For example,sass-compile-w-table
is nowsass-table:only
. This better expresses the purpose of these tasks: to run only the one task without dependencies for the purpose of rebuilding.Reviewers should focus on:
Understanding how it all fits together, so we can all support this going forward.
⭐️ Reading this PR commit-by-commit is encouraged. I worked hard to keep the commits separate and logical.