-
Notifications
You must be signed in to change notification settings - Fork 15
Add options to create bundles that can be lazy loaded #35
base: master
Are you sure you want to change the base?
Conversation
…lazy-loaded bundles.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
I signed it! |
CLAs look good, thanks! |
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.
Thank you for the contribution! This seems like a really useful feature.
I found a couple minor things that need tweaking and I had a couple questions.
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.
LGTM. A few minor nits.
Thanks for the contribution!
README.md
Outdated
@@ -33,7 +33,9 @@ The build tools also include a CLI, which can be used by installing the tools gl | |||
| ----------------------- | ----------- | -------------------------------------------------------------------------- | | |||
| `destination` (*) | `string` | Target location for the Material build. | | |||
| `modules` | `string[]` | Modules that should be part of the build.<br/> All modules will be built if nothing is specified. | | |||
| `version` | `string` | Version of AngularJS Material.<br/> If set to `local`, it will take the local installed AngularJS Material version from the node modules. <br/> If set to `latest`, the latest version will be downloaded. | | |||
| `excludeModules` | `string[]` | Modules that should be excluded from the build.<br/> Use to exclude e.g. core modules when building a bundle that should get lazy-loaded. | |
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 should mention that this is not a full string match, but it also matches partially. maybe an example as you already added in code? (animate
=> material.core.animate
)
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 agree that this would be a helpful addition as well.
| `theme` | `MdTheme` | Material Theme to be used to generate a static theme stylesheet. | | ||
| `themes` | `MdTheme[]` | Multiple Material Themes, which are used to generated a static stylesheet. | | ||
| `cache` | `string` | Directory for caching the downloads | | ||
| `mainFilename` | `string` | Name of the entry file that will be loaded to figure out the dependencies. | | ||
| `destinationFilename` | `string` | Name to be used as a base for the output files. | | ||
| `excludeModules` | `string[]` | Modules that should be excluded from the build.<br/> Use to exclude e.g. core modules when building a bundle that should get lazy-loaded. | | ||
| `excludeMainModule` | `boolean` | Set to `true` to exclude the code for the Angular Material main module. Use when building a bundle that will get lazy-loaded and extend an already existing main module. | |
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.
There seems to be a lot of extra space here after the name and before the type.
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.
Please change Angular to AngularJS.
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 version
has been also changed from AngularJS Material to Angular Material 😄
Please fix that issue and also mention that the excludeModules
option "partially" matches modules. This would be ready then!
@@ -33,12 +33,14 @@ The build tools also include a CLI, which can be used by installing the tools gl | |||
| ----------------------- | ----------- | -------------------------------------------------------------------------- | | |||
| `destination` (*) | `string` | Target location for the Material build. | | |||
| `modules` | `string[]` | Modules that should be part of the build.<br/> All modules will be built if nothing is specified. | | |||
| `version` | `string` | Version of AngularJS Material.<br/> If set to `local`, it will take the local installed AngularJS Material version from the node modules. <br/> If set to `latest`, the latest version will be downloaded. | | |||
| `version` | `string` | Version of AngularJS Material.<br/> If set to `local`, it will take the local installed Angular Material version from the node modules. <br/> If set to `latest`, the latest version will be downloaded. | |
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.
AngularJS is the correct form to use here. Please revert.
Just wanted to drop a note here to remind you that there are some outstanding changes needed to this PR before we can merge it. Thank you. |
Add two new options
--exclude-modules
and--exclude-main-module
that do exactly that.The reasoning behind this is that I want to be able to break up Material into a lean base bundle that contains core and a few other necessary modules for the initial app load. Accompanied by more bundles that can be lazy-loaded upon demand. These lazy-loaded bundles however must not include the core modules again.