-
Notifications
You must be signed in to change notification settings - Fork 6.8k
build: use shared logic to build packages #4202
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,21 @@ | ||
import {createPackageBuildTasks} from './util/package-tasks'; | ||
|
||
/** 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's just call this one There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name in the createPackageBuildTasks('material-examples', 'examples', ['material']); But this just causes confusion because the package and task name are different. Actually the same for We should try to be as consistent as possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
import './tasks/ci'; | ||
import './tasks/clean'; | ||
import './tasks/default'; | ||
import './tasks/development'; | ||
import './tasks/docs'; | ||
import './tasks/e2e'; | ||
import './tasks/lint'; | ||
import './tasks/release'; | ||
import './tasks/publish'; | ||
import './tasks/screenshots'; | ||
import './tasks/unit-test'; | ||
import './tasks/aot'; | ||
import './tasks/payload'; | ||
import './tasks/coverage'; | ||
import './tasks/library'; | ||
import './tasks/examples'; | ||
import './tasks/cdk'; | ||
import './tasks/material-release'; |
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
import {task, src, dest} from 'gulp'; | ||
import {join} from 'path'; | ||
import {writeFileSync} from 'fs'; | ||
import {Bundler} from 'scss-bundle'; | ||
import {execNodeTask, sequenceTask} from '../util/task_helpers'; | ||
import {composeRelease} from '../util/package-build'; | ||
import {COMPONENTS_DIR, DIST_MATERIAL, DIST_RELEASES} from '../constants'; | ||
|
||
// There are no type definitions available for these imports. | ||
const gulpRename = require('gulp-rename'); | ||
|
||
// Path to the release output of material. | ||
const releasePath = join(DIST_RELEASES, 'material'); | ||
// The entry-point for the scss theming bundle. | ||
const themingEntryPointPath = join(COMPONENTS_DIR, 'core', 'theming', '_all-theme.scss'); | ||
// Output path for the scss theming bundle. | ||
const themingBundlePath = join(releasePath, '_theming.scss'); | ||
// Matches all pre-built theme css files | ||
const prebuiltThemeGlob = join(DIST_MATERIAL, '**/theming/prebuilt/*.css'); | ||
// Matches all SCSS files in the library. | ||
const allScssGlob = join(COMPONENTS_DIR, '**/*.scss'); | ||
|
||
/** | ||
* Overwrite the release task for the material package. The material release will include special | ||
* files, like a bundled theming SCSS file or all prebuilt themes. | ||
*/ | ||
task('material:build-release', ['material:prepare-release'], () => composeRelease('material')); | ||
|
||
/** | ||
* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
'material:build', | ||
['material:copy-prebuilt-themes', 'material:bundle-theming-scss'] | ||
)); | ||
|
||
/** Copies all prebuilt themes into the release package under `prebuilt-themes/` */ | ||
task('material:copy-prebuilt-themes', () => { | ||
src(prebuiltThemeGlob) | ||
.pipe(gulpRename({dirname: ''})) | ||
.pipe(dest(join(releasePath, 'prebuilt-themes'))); | ||
}); | ||
|
||
/** Bundles all scss requires for theming into a single scss file in the root of the package. */ | ||
task('material:bundle-theming-scss', () => { | ||
// Instantiates the SCSS bundler and bundles all imports of the specified entry point SCSS file. | ||
// A glob of all SCSS files in the library will be passed to the bundler. The bundler takes an | ||
// array of globs, which will match SCSS files that will be only included once in the bundle. | ||
new Bundler().Bundle(themingEntryPointPath, [allScssGlob]).then(result => { | ||
writeFileSync(themingBundlePath, result.bundledContent); | ||
}); | ||
}); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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
Uh oh!
There was an error while loading. Please reload this page.
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.
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.