build: use shared logic to build packages #4202
Conversation
* No longer creates the whole set of gulp tasks just to build a single package. * Makes the package building & releasing more consistent. No extra `build:release` just for the `lib` package. References angular#4108
googlebot
added
the
cla: yes
| @@ -8,7 +8,7 @@ | |||
| "url": "https://github.com/angular/material2.git" | |||
| }, | |||
| "scripts": { | |||
| "build": "gulp build:release", | |||
| "build": "gulp material:build-release:clean", | |||
There was a problem hiding this comment.
Doesn't has to happen in this PR, but can we swap the rule names so that they're consistently verb:subject again? E.g.,
build:material
watch:material
serve:devapp
release:cdk
test:cdk
package:material (create the release package)
publish:material (publishes to npm)
clean:cdk
It would also be good (in a follow-up PR) to have just gulp build, gulp test, etc. that do material, cdk, and examples.
There was a problem hiding this comment.
Not sure about that. It's very nice to have like all tasks that belong to a different "package" / "group" be prefixed with the actual name of it.
cdk:clean-build
cdk:build
cdk:build-release
cdk:watchThere was a problem hiding this comment.
I think of it as "I want to perform some action. That action will affect xxx". Overall I like the consistency of "verb:action" more than organizing the tasks by target
There was a problem hiding this comment.
Yeah I see that it might be nicer when building different packages on their own. I personally vote for organizing the tasks by target.
It just feels not very consistent and convenient when naming stuff that won't be available in other tasks.
copy-assets:demo-app.bundle-theme-scss:material
I would be fine revisiting later in another PR or issue.
| /** Create gulp tasks to build the different packages in the project. */ | ||
| createPackageBuildTasks('cdk'); | ||
| createPackageBuildTasks('material', ['cdk']); | ||
| createPackageBuildTasks('material-examples', ['material']); |
There was a problem hiding this comment.
Let's just call this one 'examples' for brevity
There was a problem hiding this comment.
The name in the package.json will be @angular/material-examples. Before I went with material-examples I had another property on createPackageBuildTasks that allows you to specify the task-prefix.
createPackageBuildTasks('material-examples', 'examples', ['material']);But this just causes confusion because the package and task name are different. Actually the same for lib (= material).
We should try to be as consistent as possible.
There was a problem hiding this comment.
The project/repo is already called "material", though; I'd say that part of the name is implicit. As someone working on it, my mental model is just "I want to build the examples" -> "build:examples".
There was a problem hiding this comment.
I totally see what you mean. My only concern is that it is very confusing to have a mismatch of package-name / task-name / source directory name.
tools/gulp/tasks/material-release.ts
Outdated
| )); | ||
|
|
||
| /** Copies all prebuilt themes into the release package under `prebuilt-themes/` */ | ||
| task('material:prebuilt-themes', () => { |
There was a problem hiding this comment.
I'd like to have the name communicate that this doesn't build the themes, just that it copies / packages them up for the release.
(same for 'material:theming-scss')
| * Task that will build the material package. It will also copy all prebuilt themes and build | ||
| * a bundled SCSS file for theming | ||
| */ | ||
| task('material:prepare-release', sequenceTask( |
There was a problem hiding this comment.
How do you feel about naming tasks that are not meant to be called from the command line with a leading :?
There was a problem hiding this comment.
We had that before for a lot of different tasks. But I just don't feel it's necessary. Just makes it harder to remind, if something is "supposed" to be called from the CLI or not.
tools/gulp/util/package-tasks.ts
Outdated
| // There are no type definitions available for these imports. | ||
| const inlineResources = require('../../../scripts/release/inline-resources'); | ||
|
|
||
| export function createPackageBuildTasks(packageName: string, requiredPackages: string[] = [], ) { |
There was a problem hiding this comment.
Add function description (which should include a list of the tasks created by the function)
jelbourn
added
pr: lgtm
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
build:releasejust for thelibpackage.Note: Madge, Payload, Publish Artifacts will be updated in a follow-up.
References #4108