Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

docs: update README with additional framework integration library #1420

Merged
merged 3 commits into from
Nov 2, 2017

Conversation

gjdev
Copy link
Contributor

@gjdev gjdev commented Oct 13, 2017

No description provided.

@codecov-io
Copy link

codecov-io commented Oct 13, 2017

Codecov Report

Merging #1420 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1420   +/-   ##
=======================================
  Coverage   99.82%   99.82%           
=======================================
  Files          69       69           
  Lines        3392     3392           
  Branches      424      424           
=======================================
  Hits         3386     3386           
  Misses          6        6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e36735b...ce10be5. Read the comment docs.

@amrtgaber
Copy link
Contributor

Hey @gjdev , thanks for taking the time to integrate MDC-Web into Angular!

I started taking a look at Blox Material to evaluate if it met our criteria but unfortunately I couldn't make it through the Getting Started guide successfully. After following the steps outlined here, the project still failed to compile. Here is the related issue I created on bitbucket.

Please follow up on that issue and then we can take another look, thanks!

@gjdev
Copy link
Contributor Author

gjdev commented Oct 23, 2017

Thanks for looking at this! There was a typo in the getting started guide, I've updated the bitbucket issue with more information.

@gjdev
Copy link
Contributor Author

gjdev commented Oct 23, 2017

FYI: we added another guide with some extra steps to demonstrate how to make a production build of the Angular App. If you've followed both guides, a (production) build of the App will only contain javascript code for Material Design Components that are actually used inside the app. So this should demonstrate the "à-la-carte delivery model" of Blox Material (one of the requirements your documentation gives for including a reference to our integration library).

@amrtgaber
Copy link
Contributor

Great job on fixing the getting started documentation! Looks great @gjdev.

Another question for you: Is the documentation provided on https://blox.src.zone/material#/directives comprehensive of all the directives that you've implemented so far?

Aside from that, I have a concerns about the way Blox Material meets the "à-la-carte delivery model" requirement. Although I see your earlier point about unused component code being compiled out, that's only true for developers who are using angular-cli and only for production builds. Unfortunately that is too narrow to meet the requirement, since anyone can import the Blox Material module whether they are using angular-cli or not. Is there a way for you to split up the delivery model? You may have already seen the Preact integration but I would recommend taking a look at that if you haven't already: https://github.com/prateekbh/preact-material-components.

@gjdev
Copy link
Contributor Author

gjdev commented Oct 24, 2017

Great job on fixing the getting started documentation! Looks great @gjdev.

Thanks!

Another question for you: Is the documentation provided on https://blox.src.zone/material#/directives comprehensive of all the directives that you've implemented so far?

We currently miss documentation for the following integrated components: form-field, ripple, and snackbar. A full list of what we have, what we are missing, and what's documented can be found here: https://bitbucket.org/src-zone/material/overview.

Aside from that, I have a concerns about the way Blox Material meets the "à-la-carte delivery model" requirement. Although I see your earlier point about unused component code being compiled out, that's only true for developers who are using angular-cli and only for production builds. Unfortunately that is too narrow to meet the requirement, since anyone can import the Blox Material module whether they are using angular-cli or not. Is there a way for you to split up the delivery model? You may have already seen the Preact integration but I would recommend taking a look at that if you haven't already: https://github.com/prateekbh/preact-material-components.

Sorry for the lengthy answer, but because of the time difference in our communication, I wanted to give as much information as I could to motivate why things are designed the way they are for Blox Material:

I took a look at Preact Material Components, as you suggested. They deliver their components in a very similar way as we do. That is, they expose one big es5 bundle under the "main" entrypoint in their package.json (which we do exactly the same). And they expose one es-modules based entrypoint under the "module" section in their package.json (which we also do, but ours is a bundled one, theirs is the original unprocessed source). Now according to convention that tools like webpack, rollup, angular-cli, etc. all agree upon, the "module" entrypoint should actually be fully es5 compatible, except for also using the es2015 module syntax (sometimes referred to as es+modules or esm). Preact Material Components breaks that convention, which is an issue for such tools. (BTW: Material Components for the Web breaks the "main" should be es5 syntax rule, which is equally inconvenient, see eg. #929 and #1227). Now for Preact Material Components it probably doesn't matter that much, as they advocate not using the standard "module"entrypoint anyway, but instead to import components directly from the directory in which they are implemented. The reason why they suggest not using the established convention is pretty well explained in their documentation https://material.preactjs.com/:

If you are using an ES6 codebase, consider using the components individually, as none of the tree shaking currently removes unused classes.

Meaning, it's a workaround for limitations of the tree-shaking algorithms. As you have seen from following our getting started guide, that limitation does not exist with the current state of tooling in combination with the way we deliver Blox Material. There are many advantages to not having to use workarounds like this one. Basically all bundlers (e.g. webpack, rollup), compilers (e.g. typescript), and transpilers (e.g. babel), work with the "main" & "module" package conventions. Virtually all libraries in the Angular ecosystem use it (e.g. Rxjs, @angular/*, @covalent/*), lots of other js libraries also do this (e.g. three.js, d3, react). I think it would be a bad idea for us to deviate from that, unless we run into the same tree-shaking issues that Preact Material Components gives as their reason to do this.

Although I see your earlier point about unused component code being compiled out, that's only true for developers who are using angular-cli

That's not true. If you are using any of the other popular devstacks for building Angular applications, it's easy to get them same results. Angular CLI actually uses webpack under the hood, and either one is what most devs use for their Angular projects. With rollup or other bundlers, you can achieve the same. Our demosite uses webpack, not Angular CLI. Please note that an angular application is very different from using material components for the web directly. Because you're using typescript, you are going to use a devstack with at least the typescript compiler, a minifier and a bundler. Many of the tree shaking improvements actually come from the Angular team, because most of the angular core modules are pretty large bundles, and splitting them up in smaller ones has a lot of disadvantages (both for library maintainers and for library consumers).

... and only for production builds

Or any other builds that you configure for tree-shaking and minification. Which you could also do for builds intended for e.g. unit tests. Or even for debug/dev build if you want to. But for debug/dev builds you really don't want to do this. With debugging you typically fire up a devserver, that monitors changes to your source files, and recompiles/processes and rebundles them. Since the third party libraries required with an angular app are BIG, and you also need the include their sourcemaps, which are HUGE, you configure that in such a way that any third-party stuff gets into a 'vendor' bundle. That way for a source code change, the big vendor 'bundle' doesn't have to be rebuild, and your devserver can almost instantly serve the new application sources. If you're going to tree-shake that big vendor bundle, than any source code change may affect the vendor bundle, which therefore has to be rebuild for any change. Being so big, that makes you wait after every code change, thus hinders development and debugging your application.

Is there a way for you to split up the delivery model?

We already put split up javascripts under the build directory of our in our npm package. Meaning you already can do your imports in the same way as advocated by that Preact integration library, and import individual directives directly from there. But you really don't want to do this for all the reasons stated above. It will also only work for Angular JIT compilation, not for Angular AOT compilation, as that needs metadata around the directives that is delivered by metadata attached to the MaterialModule. So it's not really a viable option for a production build anyway. The only way I have seen Angular libraries split up their package in the way you seem to suggest, is how the @angular/material package (https://material.angular.io/) does it: They write additional code to expose any component as a separate angular module. They then ask their consumers to write their own angular module that just imports and exports the split-up modules they want to use, and then import that custom module in their application. The reason @angular/material did this is the same as the reason quoted above from the Preact integration (quote from the @angular/material changelog):

We've found that, with the current state of tree-shaking in the world, that using an aggregate NgModule like MaterialModule leads to tools not being able to eliminate code for components that aren't used.

That was more than 6 months ago, and at the same time angular teams started to improve the tree-shaking algorithms around Angular apps, which we can currently use to not have to resort to tricks like this.

So for a short summary answer: we could technically do the same as the @angular/material team does, by exposing many small modules, instead of one bigger module. This will make consuming the library harder, will make the typical development/debug cycle slower, will make a production build actually slightly bigger because of the extra angular modules code required, and will have no advantages that I can see. Unless I am missing something here, I think it would be a bad decision for us to do that.

I also honestly believe that what we do is actually more "a la carte" for an Angular app than such a solution, because it doesn't require extra boilerplate module code from library consumers. Just use the directives you want, and they will be the only ones delivered to your application code.

@amrtgaber
Copy link
Contributor

amrtgaber commented Oct 24, 2017

Thanks for the thorough and well thought out response @gjdev. The effort is very much appreciated 👍 You brought up several great points. Please give us a little time to decide how to proceed.

Also, thanks for clarification on the documentation.

@lynnmercier
Copy link
Contributor

Hey @gjdev , thanks again for the very thorough response.

I do have one other concern I hope you can help me understand. Right now MDC Web is in alpha version, so we making breaking changes without updating our major version.

But in some (hopefully near) future we will reach a Beta version. At which point we need to be able to make breaking changes to an individual component, and update that individual component's major version.

I'm not entirely sure I understand how Angular handles versioning. But what is the plan in the future to handle breaking changes to individual components?

@gjdev
Copy link
Contributor Author

gjdev commented Nov 2, 2017

Hi @lynnjepsen thanks for looking in to this again! I don't think how we handle breaking changes is going to be different from other integration libraries that are published as one npm package. (e.g. the Preact Material Components library that was mentioned before as an examples for the 'a la carte' requirement.)

There's a couple of things that can happen when an individual upstream material component introduces a breaking change:

  • Some breaking changes won't affect Blox Material. E.g. when the change is to sass mixins (like the ripple mixins in the last release), Blox Material is not affected at all. In those cases users can just update the individual material component, once they upgrade their own code to the new mixins. Blox Material may want to declare compatibility with additional semver ranges for that material component in a new release though.
  • When the breaking change is in a foundation api, or adapter interface, it does affect our integration library. But in some cases we may be able to support both versions in a new release. For example when the name of an adapter method is changed, we can probably support both versions, by implementing the adapter method with both names. In that case we can release a new version supporting both versions of the upstream material component. That would introduce a new release of Blox Material that is backward compatible. Users can choose which version of the upstream material component they install.
  • In cases were the upstream change forces breaking changes in angular blox, we'll upgrade, mark this as a breaking change, and change our version accordingly. In that case the new version of our library will not be compatible with older versions of the upstream material component. So in that case, users of Blox Material will be forced to upgrade to the newer version of the material component, if they want to upgrade Blox Material.

Does that answer your question?

@lynnmercier
Copy link
Contributor

It does answer my questions. Thanks @gjdev . Lets merge this!

Copy link
Contributor

@lynnmercier lynnmercier left a comment

Choose a reason for hiding this comment

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

LGTM

@lynnmercier lynnmercier merged commit 0ba2c57 into material-components:master Nov 2, 2017
@gjdev
Copy link
Contributor Author

gjdev commented Nov 3, 2017

Great, thank you all!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants