Skip to content

build: create JS bundles for each component #6998

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

Merged
merged 1 commit into from
Sep 14, 2017

Conversation

jelbourn
Copy link
Member

@jelbourn jelbourn commented Sep 12, 2017

Before this change, all of @angular/material was bundled into a
single, flat JS file.

After this change, each top-level component (i.e., each directory under
src/lib) will be have its own JS file (and d.ts and metadata.json).

The benefit of this change is that current versions of webpack will be
able to do a much better job of only including the code an app is using.

It also allows (but does not require) people to explicitly import from specific components,
e.g.

import {MdButtonModule} from '@angular/material/button';

For development within Angular Material itself we have to use these
deeper imports because the root @angular/material doesn't exist until
the actual package is created.

Additionally, this PR adds a tsconfig.json for each dir under src/
with the correct path mappings for that directory, so that IDEs will
correctly generate imports.

Closure Compiler CI check is also disabled by this PR; it cannot work
with the new package structure until a release with
google/closure-compiler#2600 is made available.

Fixes #4137

@jelbourn jelbourn added area: dev-infra Issue related to internal project infrastructure pr: needs review labels Sep 12, 2017
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 12, 2017
@@ -5,7 +5,7 @@
"declaration": true,
"stripInternal": false,
"experimentalDecorators": true,
"noUnusedParameters": true,
"noUnusedParameters": false,
Copy link
Member

Choose a reason for hiding this comment

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

Why are we no longer having this check? Maybe add a comment if necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

Was just trying something during development; reverted

module: `../@angular/${packageName}/${entryPointName}.es5.js`,
es2015: `../@angular/${packageName}/${entryPointName}.js`,
module: `../esm5/${packageName}/${entryPointName}.es5.js`,
es2015: `../esm2015/${packageName}/${entryPointName}.js`,
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity: How does this work with Closure Compiler now? Is there an alternative to the js_module_roots option?

Copy link
Member Author

Choose a reason for hiding this comment

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

Short answer: it doesn't.

I updated the script and left a comment explaining the current situation w/ closure.

"flatModuleId": "@angular/material",
"skipTemplateCodegen": true
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Is that file used anywhere? We now have three tsconfig-XX files in the lib/ package.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was just trying something out with this, meant to delete it.

Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

High-level LGTM

@jelbourn jelbourn force-pushed the build-bits branch 8 times, most recently from 8d1216a to f50ea1a Compare September 12, 2017 22:57
@jelbourn
Copy link
Member Author

jelbourn commented Sep 12, 2017

  • Rebased such that it includes material-moment-adapter/
  • Added a tsconfig.json per src/ directory in order to make IDEs auto-add imports to the right path.
  • Disabled closure compiler CI check temporarily (need a new release)
  • Refactored rollup bundling code into a class, PackageBundler so that it can maintain the context of the BuildPackage
  • Fixed prerender task

@jelbourn
Copy link
Member Author

@devversion after this PR we're definitely going to have to parallelize the typescript builds and bundling. With this change it takes over a minute. To paralleize the tasks, we'll need to manually spawn a child process for each, since nodejs is single-threaded. We also probably want to tweak the watch tasks to only re-compile the sub-packages that changed. Let me know if you want to work on all that.

All of this will be obsolete when we switch to bazel, but we're at least a month away from making that switch.

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.

@jelbourn That makes sense. In regards to the tsconfig per package directory, didn't the tsconfig in the project root work anymore?

Also LGTM! Nice

@jelbourn
Copy link
Member Author

@devversion the tsconfig in the root would cause IDEs to autogenerate the wrong imports sometimes.

Before this change, all of `@angular/material` was bundled into a
single, flat JS file.

After this change, each top-level component (i.e., each directory under
`src/lib`) will be have its own JS file (and d.ts and metadata.json).

The benefit of this change is that current versions of webpack will be
able to do a much better job of only including the code an app is using.

It also allows (but does not require) people to explicitly import from specific components,
e.g.
```
import {MdButtonModule} from '@angular/material/button';
``

For development _within Angular Material itself_ we have to use these
deeper imports because the root `@angular/material` doesn't exist until
we build the relase package.

Additionally, this PR adds a `tsconfig.json` for each dir under `src/`
with the correct path mappings for that directory, so that IDEs will
correctly generate imports.

Closure Compiler CI check is also disabled by this PR; it cannot work
with the new package structure until a release with
google/closure-compiler#2600 is made available.
@jelbourn jelbourn added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Sep 13, 2017
@jelbourn
Copy link
Member Author

Caretaker note: both the copy config and the build rules need to updated alongside this change

@jelbourn
Copy link
Member Author

Another note: if this gets merged alongside another PR, it will potentially need a build fix (for anything importing from, e.g., ../core)

@mmalerba mmalerba merged commit 1cb8907 into angular:master Sep 14, 2017
josephperrott pushed a commit to josephperrott/components that referenced this pull request Sep 15, 2017
Before this change, all of `@angular/material` was bundled into a
single, flat JS file.

After this change, each top-level component (i.e., each directory under
`src/lib`) will be have its own JS file (and d.ts and metadata.json).

The benefit of this change is that current versions of webpack will be
able to do a much better job of only including the code an app is using.

It also allows (but does not require) people to explicitly import from specific components,
e.g.
```
import {MdButtonModule} from '@angular/material/button';
``

For development _within Angular Material itself_ we have to use these
deeper imports because the root `@angular/material` doesn't exist until
we build the relase package.

Additionally, this PR adds a `tsconfig.json` for each dir under `src/`
with the correct path mappings for that directory, so that IDEs will
correctly generate imports.

Closure Compiler CI check is also disabled by this PR; it cannot work
with the new package structure until a release with
google/closure-compiler#2600 is made available.
@jelbourn jelbourn deleted the build-bits branch April 2, 2018 22:30
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: dev-infra Issue related to internal project infrastructure cla: yes PR author has agreed to Google's Contributor License Agreement merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tree-shaking for production build is not working properly
5 participants