★ wanayoo — archive 1999 https://github.com/angular/material-tools/pull/35Nouvelle recherche | Portail wanayoo
Skip to content
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

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

Open
wants to merge 7 commits into
base: master
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.

…lazy-loaded bundles.
@googlebot
Copy link

@googlebot googlebot commented Nov 27, 2018

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

@alexthewilde alexthewilde commented Nov 27, 2018

I signed it!

@googlebot
Copy link

@googlebot googlebot commented Nov 27, 2018

CLAs look good, thanks!

@Splaktar Splaktar self-assigned this Nov 28, 2018
@Splaktar Splaktar requested a review from devversion Nov 28, 2018
Copy link
Member

@Splaktar Splaktar left a comment

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

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. |

This comment has been minimized.

@devversion

devversion Dec 10, 2018
Member

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)

This comment has been minimized.

@Splaktar

Splaktar Dec 12, 2018
Member

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. |

This comment has been minimized.

@Splaktar

Splaktar Dec 12, 2018
Member

There seems to be a lot of extra space here after the name and before the type.

This comment has been minimized.

@Splaktar

Splaktar Dec 12, 2018
Member

Please change Angular to AngularJS.

This comment has been minimized.

@devversion

devversion Dec 12, 2018
Member

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. |

This comment has been minimized.

@Splaktar

Splaktar Dec 12, 2018
Member

AngularJS is the correct form to use here. Please revert.

@Splaktar
Copy link
Member

@Splaktar 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 join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.