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

performance(modules): Only import modules that are required. (closes #287) #321

Merged
merged 21 commits into from
Feb 8, 2017

Conversation

emoralesb05
Copy link
Contributor

@emoralesb05 emoralesb05 commented Feb 4, 2017

Description

There is an issue tree shaking because of how Typescript + Webpack compile and bundle things.

angular/angular-cli#2901
webpack/webpack#2899

We do not want to depend on tree shaking alone (if they fix it great, if not doesnt matter), so we need to explicitly import the modules that are required and nothing else. #287

For now we will just assume that if you import CovalentCoreModule you want everything, but if you import a submodule then we will only use the required ones.

What's included?

  • Removal of generic MaterialModule.forRoot() imports and only adding required ones per module

e.g.

Search module only requires the following modules:

    FormsModule,
    CommonModule,
    MdInputModule.forRoot(),
    MdIconModule.forRoot(),
    MdButtonModule.forRoot(),

Test Steps

  • ng serve
  • See the docs still work with only the required modules.

General Tests for Every PR

  • ng serve --aot still works.
  • npm run lint passes.
  • npm test passes and code coverage is not lower.
  • npm run build still works.

Copy link
Contributor

@kyleledbetter kyleledbetter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all tests passed and works well

@@ -2,7 +2,7 @@ import { Type } from '@angular/core';
import { NgModule, ModuleWithProviders } from '@angular/core';

import { CommonModule } from '@angular/common';
import { MaterialModule } from '@angular/material';
import { } from '@angular/material';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the intent behind leaving this empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To see if people would notice haha jk, my bad

@@ -2,7 +2,7 @@
<md-select [(ngModel)]="value"
[placeholder]="label"
[required]="required"
flex>
flex="100">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably I am not aware, how flex is different from flex="100"

@emoralesb05 emoralesb05 merged commit 1b1b49f into develop Feb 8, 2017
@emoralesb05 emoralesb05 deleted the feature/module-optimize branch February 8, 2017 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants