Skip to content
This repository has been archived by the owner on Mar 20, 2024. It is now read-only.

Add options to create bundles that can be lazy loaded #35

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

alexthewilde
Copy link

@alexthewilde alexthewilde commented Nov 27, 2018

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.

@googlebot
Copy link

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. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@alexthewilde
Copy link
Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@Splaktar Splaktar self-assigned this Nov 28, 2018
Copy link
Member

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

README.md Outdated Show resolved Hide resolved
lib/cli/options.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@devversion devversion left a 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 Show resolved Hide resolved
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. |
Copy link
Member

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)

Copy link
Member

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.

lib/builders/MaterialBuilder.ts Show resolved Hide resolved
| `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. |
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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. |
Copy link
Member

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.

@Splaktar
Copy link
Member

Splaktar commented Jan 9, 2020

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.

@Splaktar Splaktar added this to the Backlog milestone Oct 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants