Skip to content

Proposal: Auto add (providers, directives, pipes, styles, styleUrls) when adding 3rd party libs, etc. #96

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

Closed
NathanWalker opened this issue Dec 9, 2015 · 46 comments

Comments

@NathanWalker
Copy link
Contributor

Discussion on AngularAir today regarding 3rd party libraries (https://plus.google.com/events/c7m79d5o04nom5eednk5ecqtebk) brought up an interesting thought.
See also: angular/angular#5503

If 3rd party libs all adhered to a certain naming convention and style when exporting necessary services, directives, pipes, etc., it would be incredibly helpful if this CLI could auto-annotate the src code files with the necessary directives, providers, pipes, etc.

Consider the following scenario:

  1. User wants to use ng2-image-lazy-load (https://github.com/NathanWalker/ng2-image-lazy-load)
  2. Since it followed certain conventions, IMAGELAZYLOAD_PROVIDERS, the CLI might provide an additional helpful aid (command can be whatever, just using add-lib for example):
ng add-lib ng2-image-lazy-load 

It would proceed to run npm install ng2-image-lazy-load, then prompt with a couple questions:

$ Would you like this provider as part of the bootstrap? (y/n)

If "y":

$ Please provide the path to the src file which bootstraps your app:
$ src/bootstrap.ts

It would add the import statements and append the aptly named [lib_prefix]_PROVIDERS in the user's app bootstrap.
If the lib contained no constants with PROVIDERS in the name, it would modify nothing.

If "n":

$ Please provide the path of the component you would like the providers added to:
$ src/components/main.component.ts

It would add the import statements and append the libraries aptly named PROVIDERS in the viewProviders of the component.

Similar scenarios would apply for DIRECTIVES, PIPES, etc.

If this were possible, every 3rd party lib could suggest using the CLI as the first option to install, followed by manual npm install.

/cc @jvandemo, @gdi2290, @valorkin, @ocombe, @jeffwhelpley, @PascalPrecht, @mgechev, @ludohenin, @robwormald

@NathanWalker NathanWalker changed the title Auto add PROVIDERS, DIRECTIVES, PIPES when adding 3rd party libs, etc. Proposal: Auto add PROVIDERS, DIRECTIVES, PIPES when adding 3rd party libs, etc. Dec 9, 2015
@IgorMinar
Copy link
Contributor

the proposal sounds reasonable to me (with some tweaks). I think that we should prefer local import over global import, so I'd reorder the options.

Would you like to create a test package(s) in npm that we can experiment with?

@NathanWalker
Copy link
Contributor Author

adjective: adlib; adverb: adlib
_musical context_:

Jazz musicians may adlib during a performance, meaning they are free to vary tempo and instrumentation.

_programmers context_:

When using any framework to create an application, there will come a time when a developer must adlib to complete the applications specifications.

Might be a fun command: adlib

@robwormald
Copy link
Contributor

i think perhaps a simple solution for this would be something along the lines of

//myPluginRoot.ts
import {MyPluginServiceA, MyPluginServiceB} from './services';
import {MyDirectiveA, MyDirectiveB} from './directives';
import {MyPipeA, MyPipeB} from './pipes';

//export the named stuff for granular usage
export * from './services'
export * from './directives'
export * from './pipes'

//use es6 default syntax to export the "plugin"
export default {
  providers: [MyPluginServiceA, MyPluginServiceB],
  directives: [MyDirectiveA, MyDirectiveB],
  pipes: [MyPipeA, MyPipeB]
}

users and tooling are then easily able to do :

//naming is irrelevant here
import myPlugin from 'myPlugin'

//now its consistent:
myPlugin.directives
myPlugin.services
myPlugin.pipes

and this is easily done with templating, as the actual name of the export is "default" - no weird lookups necessary and no NAMING_CONVENTIONS to force / invent.

see http://plnkr.co/edit/Yi1e3ifiX0gSqd8Qs2h4?p=preview - i added a little plugin registration helper too, which @IgorMinar will probably yell at me about.

@NathanWalker
Copy link
Contributor Author

That's the best solution I've heard yet @robwormald. Very clean and simple.

I could pull your plunk (the gist of it anyway) into a repo tomorrow or
Friday and publish it so it could be experimented with in this setup.

@ludohenin
Copy link

@robwormald I like it very much

@ocombe
Copy link
Contributor

ocombe commented Dec 10, 2015

+1 on this solution @robwormald :)

@jvandemo
Copy link

@robwormald — Brilliant, love your proposal! 👍

@robwormald
Copy link
Contributor

Go for it! (maybe not the helper thing...)

On Dec 10, 2015, at 12:16 AM, Nathan Walker [email protected] wrote:

That's the best solution I've heard yet Rob. Very clean and simple.

I could pull your plunk (the gist of it anyway) into a repo tomorrow or
Friday and publish it so it could be experimented with in this setup.

On Thu, Dec 10, 2015 at 12:03 AM Rob Wormald [email protected]
wrote:

i think perhaps a simple solution for this would be something along the
lines of

//myPluginRoot.tsimport {MyPluginServiceA, MyPluginServiceB} from './services';import {MyDirectiveA, MyDirectiveB} from './directives';import {MyPipeA, MyPipeB} from './pipes';
//export the named stuff for granular usageexport * from './services'export * from './directives'export * from './pipes'
//use es6 default syntax to export the "plugin"export default {
providers: [MyPluginServiceA, MyPluginServiceB],
directives: [MyDirectiveA, MyDirectiveB],
pipes: [MyPipeA, MyPipeB]
}

users and tooling are then easily able to do :

//naming is irrelevant hereimport myPlugin from 'myPlugin'
//now its consistent:
myPlugin.directives
myPlugin.services
myPlugin.pipes

and this is easily done with templating, as the actual name of the export
is "default" - no weird lookups necessary and no NAMING_CONVENTIONS to
force / invent.

see http://plnkr.co/edit/Yi1e3ifiX0gSqd8Qs2h4?p=preview - i added a
little plugin registration helper too, which @IgorMinar
https://github.com/IgorMinar will probably yell at me about.


Reply to this email directly or view it on GitHub
#96 (comment).


Reply to this email directly or view it on GitHub #96 (comment).

@valorkin
Copy link
Contributor

Will not this push to use MVC like folder separation?
Like for angular 1 really was really popular controllers\directives\templates separation
which became really fast unusable in large apps
that all moved to components separation all related to component in one folder
a bit later nested components and common folders appeared as a practice

+1 to @robwormald that we should not push/invent name conventions

PS: I am never using generators, and not allow to, guys should understand what and why they are doing :)

@robwormald
Copy link
Contributor

this is for third party packages (like yours) which will presumably ship via NPM, and thus the structure of the package is mostly irrelevant to the user. The structure internal to the module / plugin is entirely up to you, that was just a quick and dirty demo.

@jvandemo
Copy link

@valorkin — I think using a folder structure with directives, services and pipes makes sense for libraries (the Angular team also does this internally in the Angular source code) although the library author could decide to use another structure.

The structure for an actual application (not a library) would be component driven though, if that makes sense.

@valorkin
Copy link
Contributor

just said my IMHO
Angular team actually uses components structure (but call it modules) and MVC separation inside of it
https://github.com/angular/angular/tree/master/modules/angular2/src

@jvandemo
Copy link

@valorkin — I think the Angular repository (in its current form) is hard to compare to a third party library.

However, if we peek inside one of the modules, then we can see a similar structure e.g.: https://github.com/angular/angular/tree/master/modules/angular2/src/common

I think such a structure is probably closer to what most (smaller) libraries will contain.

If you have a larger library that contains modules then it definitely makes sense to create a module folder structure. Ultimately the library author should decide.

Although one could argue that it would be better to split into different repositories than in different modules.

Would be great if there is an Angular 2 ecosystem with a lot of very small libraries (as in node).

What are your thoughts on splitting into different repositories? Thanks!

@valorkin
Copy link
Contributor

Taking in account user option to to require and bundle only require part of libs
it is awesome opportunity to minimize js size and bootstrap time
(as a consequence of reduced amount of js to be parsed by VM)

modules are optional
splitting in components like this: https://github.com/angular/angular/tree/master/modules/angular2/src
seems to me as proven concept, IMHO

@NathanWalker
Copy link
Contributor Author

@IgorMinar Here is a test lib to experiment with that uses @robwormald's excellent standards proposal above:
https://github.com/NathanWalker/ng2-cli-test-lib

npm install ng2-cli-test-lib

Expectations of cli

  • manually npm installing may not be required (cli could handle that if it makes sense to do so)
  • questions should be prompted similar to suggestions above to allow the user to specify how they would like the lib added and annotated in their src
  • cli should properly add the correct annotations given the lib's standard default exports

Sample component to test if it worked

@Component({
    selector: 'demo',
    template: `
      <test></test>
      <div>{{title | uppercase}}</div>
    `,
    directives: [.... **hopefully cli will auto add these properly** ...],
    pipes: [.... **hopefully cli will auto add these properly** ...]
})
export class Demo {
    public title:string = "having standards for these libs will help a lot!";

    constructor(private test1:TestService, private test2:TestService2) { }
}

bootstrap(Demo, [.... **hopefully cli will auto add these properly** ...])

Given something like the above, you should be able to see this in the console:
screen shot 2015-12-11 at 12 33 01 am

TestService and TestService2 simply log to console that they have been created properly via DI.
TestPipe lets us know it's transforming successfully.

In the browser, this should be seen:
screen shot 2015-12-11 at 12 34 43 am

The TestDirective in the lib simply inserts text into the element.

The TestPipe simply transforms title to uppercase.

Conclusions

  • If all this works out ok, an official styleguide could be written regarding the authoring of 3rd party libs for Angular2. It would also suggest all lib's mention angular-cli as the primary method to add the lib.

btw, @robwormald's registerPlugins (in pluginHelper.ts) utility found here http://plnkr.co/edit/Yi1e3ifiX0gSqd8Qs2h4?p=preview is pretty nifty 👍

@NathanWalker NathanWalker changed the title Proposal: Auto add PROVIDERS, DIRECTIVES, PIPES when adding 3rd party libs, etc. Proposal: Auto add providers, directives, pipes when adding 3rd party libs, etc. Dec 11, 2015
@valorkin
Copy link
Contributor

@NathanWalker you just made best ng2 starter package for angular 2? You know it right? :)

@jvandemo
Copy link

@NathanWalker — Great effort! Thanks!

Once we have settled on a best practice I will also update generator-angular2-library accordingly.

@NathanWalker
Copy link
Contributor Author

fyi, testing lib https://www.npmjs.com/package/ng2-cli-test-lib, updated to use latest alpha.53.

@IgorMinar @robwormald if either of you have any opinion on lib name prefix (repo prefix), please share. Most of the discussion has settled on ng2- for becoming the standard prefix for all angular2 libs to come. But I think there's still time to settle on something else before the landscape starts to fill up dramatically with ng2-.

@robwormald
Copy link
Contributor

no real comment on the prefix.

thinking about the installation process - i'd personally rather use npm. i think it would be reasonable to have package authors add a field to their package.json (something like "ng" : { "plugin" : true }) - the CLI can then scan for those. needs a bit more thought on that, but I don't think we want to build a package manager too :)

@NathanWalker
Copy link
Contributor Author

I was thinking the same. Using npm manually is likely universally preferred.
My thought initially on that was more for the pure newbie (new to node, new to angular, etc.) and since it might be rather trivial, it might make sense. The teams best discretion there.

@PatrickJS
Copy link
Contributor

you should also add styles

import ng2lib, {MyDirective} from 'ng2lib';
@Component({
  selector: 'app',
  providers: [  ...ng2lib.providers],
  directives: [  ...ng2lib.directives],
  styles: [ ...ng2lib.styles],
  template: `<my-directive></my-directive>`
})
export class App {}

parsing package.json for the correct keyword angular2, ng2 or, ng2module (whatever everyone agrees on) should be prefered when segmenting npm for example npm search for ng2 (tbh they should really add hashtags to npm)

@NathanWalker
Copy link
Contributor Author

+1, fully integrated :)

@jvandemo
Copy link

What if a library author would configure package.json to run angular-cli as an npm postinstall script?

Possible benefits:

  • angular-cli would automatically run after npm install
  • library author can configure angular-cli to run with options specific to the library
  • angular-cli itself does not have to be aware of anything, it is the library author that calls angular-cli
  • backwards compatible: no postinstall script => no action is taken
  • expandable: the library author could perform other actions as well in the postinstall script
  • flexible: library author can choose preferred language e.g. JavaScript or bash
  • script can be standalone run using npm run postinstall

Possible issues:

  • What if angular-cli is not installed? Should angular-cli be a devDependency?
  • Script should be non-interactive to ensure that it can be run unattended e.g. on Travis?

@ocombe
Copy link
Contributor

ocombe commented Dec 14, 2015

What happens when the end user doesn't want the lib to auto install inside its application?

@NathanWalker
Copy link
Contributor Author

@ocombe In that case, user could simply exit, Ctrl + c, since the install would have completed, they could just exit the postinstall angular cli if they didn't want to go through those additional steps.

@jvandemo great additional thoughts. Using cli manually could still work as suggested/described throughout this discussion and as an option on top of everything, libs could also provide this postinstall step as a convenience. I think having it both ways (via angular cli directly or via postinstall when using npm manually to install a lib) doesn't hurt.

Possible issues:

  • What if angular-cli is not installed? Should angular-cli be a devDependency?
    I vote no. It just wouldn't que up angular-cli if it's not installed, or may report a warning after installing. Either way, the lib would describe in its installation steps that installing angular-cli as a prerequisite may help them configure their app to use the lib but it's not required.
  • Script should be non-interactive to ensure that it can be run unattended e.g. on Travis?
    This alone may make having a postinstall step problematic. But if there's an easy way to turn off for Travis, then it would be fine.

@NathanWalker
Copy link
Contributor Author

Based on @gdi2290 suggestion to handle styles, I've updated the test lib to handle this as well.

This works out like the following:

import {TestDirective} from './src/app/directives/test.directive';
import {TestService, TestService2} from './src/app/providers/test.provider';
import {TestPipe} from './src/app/pipes/test.pipe';
import {TestStyles} from './src/app/test.styles';

export * from './src/app/directives/test.directive';
export * from './src/app/providers/test.provider';
export * from './src/app/pipes/test.pipe';
export * from './src/app/test.styles';

export default {
  directives: [TestDirective],
  pipes: [TestPipe],
  providers: [TestService, TestService2],
  styles: TestStyles.styles(),
  styleUrls: ['src/public/css/test.css']
}

This attempts to handle styles and styleUrls just to experiment with.
styles utilizes a service with a static method of inline styles that the lib uses:

export class TestStyles {
  public static styles(): string[] {
    return [`
      test a {
        font-size:20px;
        color:blue;
      }
    `];
  }
}

This may or may not be the best way forward, but wanted to give you something to experiment with here to handle inline styles as well as styleUrls.

Test if styles were added properly

If all the styles load properly (the inline as well as the style urls), then the directive should render in blue with font-size:20px (from the inline style the TestStyles service provides) like so:
screen shot 2015-12-14 at 8 35 58 pm

Additionally if the styleUrls load properly (from 'src/public/css/test.css'), the :hover state should be red with text-decoration:none and use cursor:pointer like so:
cli

styleUrls considerations

For this to work, a couple things will probably need to happen:

  • the cli would likely need to prompt the user asking for their public directory to copy the css file to
  • or the cli could prefix the styleUrls with a directory location gathered from the user upon cli questioning
  • or any combination of copying the css file and/or simply prefixing a proper directory location proper to the particular applications setup

Lastly, note that the TestDirective now inserts a link to this repo itself.

@NathanWalker NathanWalker changed the title Proposal: Auto add providers, directives, pipes when adding 3rd party libs, etc. Proposal: Auto add (providers, directives, pipes, styles, styleUrls) when adding 3rd party libs, etc. Dec 15, 2015
@ocombe
Copy link
Contributor

ocombe commented Dec 15, 2015

@ocombe In that case, user could simply exit, Ctrl + c, since the install would have completed, they could just exit the postinstall angular cli if they didn't want to go through those additional steps.

No, it should entirely be optional.
If we use it in the postinstall for npm, then I think that we should have another npm plugin that would be an angular-cli helper, this would be a dependency for your lib, and would check postinstall if angular-cli is installed in this repository, in which case it would install the lib (or offer to, we can disable this in CI environment pretty easily), and if not it would just exit.

@jvandemo
Copy link

@ocombe — No, it should entirely be optional.

Agreed, maybe any npm script that is not triggered automatically could do e.g.:

$ npm run angular-cli

Then it would be optional and the benefits of the script remain.

It would also solve the dependency issue where the consumer would have to have angular-cli installed globally to run the script (and thus angular-cli doesn't have to be a devDependency).

@jvandemo
Copy link

@NathanWalker — This attempts to handle styles and styleUrls just to experiment with. Styles utilizes a service with a static method of inline styles that the lib uses.

Great work Nathan! What is your reasoning behind using a static method for styles?

@NathanWalker
Copy link
Contributor Author

@jvandemo thanks, it's really just 1 way to do it, no particular reason,
and should be simple to test and experiment with. If you have any ideas on
some other ways to handle both inline styles and/or styleUrls, lemme know.

On Mon, Dec 14, 2015 at 11:34 PM Jurgen Van de Moere <
[email protected]> wrote:

@NathanWalker https://github.com/NathanWalker — This attempts to handle
styles and styleUrls just to experiment with. Styles utilizes a service
with a static method of inline styles that the lib uses.

Great work Nathan! What is your reasoning behind using a static method for
styles?


Reply to this email directly or view it on GitHub
#96 (comment).

@jvandemo
Copy link

@NathanWalker Interesting, because using a function could be convenient to pass in options e.g.:

styles: TestStyles.styles({
  flexbox: true
})

@NathanWalker
Copy link
Contributor Author

@jvandemo +1 would provide for some very interesting app configurations across various devices ;)

@NathanWalker
Copy link
Contributor Author

https://www.npmjs.com/package/ng2-cli-test-lib updated to 2.0.0-beta.0

@NathanWalker
Copy link
Contributor Author

Turns out a really cool guy @superchris tried out the lib today on his own brave accord and in the process helped reveal a couple things, of which I just fixed in 1.0.7. (it was previously installing its own dependencies in node_modules (within the lib's folder) after installing).

His project shows an example implementation of the ng2-cli-test-lib:
https://github.com/superchris/angular-cli-test

One thing of interest is that the ComponentMetadata:
providers, viewProviders, directives, and pipes all support nested arrays.

However styles and styleUrls do not.
I'm sure this is for good reason but maybe it should be consistent for all metadata.

Given this, when integrating the library in, I had to do this:
https://github.com/superchris/angular-cli-test/blob/master/src/app/angular-cli-test.ts#L10
Which is not 1/1 consistent with the others. Not sure if this should be reported as an issue or if this difference is warranted.

Lastly, until the cli implements some of the features aforementioned, styleUrls will be a more manual process when installing/using 3rd party libs, so I did not implement that in @superchris awesome test project.

@PatrickJS
Copy link
Contributor

we should be using spread operator whenever possible since it also clearly shows which ones are collections and which ones aren't (as if all caps wasn't enough)

import {FORM_DIRECTIVES} from 'angular2/common';
import {APP_PROVIDERS} from '../app-providers';
import {MyDirective} from './my-directive';
import {APP_STYLES, APP_THEME_STYLES} from '../app-styles';
@Component({
  selector: 'home',
  providers: [
    ...APP_PROVIDERS
  ],
  directives: [
    ...FORM_DIRECTIVES,
    MyDirective
  ],
  styles: [
    ...APP_THEME_STYLES,
    `
      .main {
        background: gray;
      }
    `
  ],
  template: `
    <my-directive></my-directive>
  `
})
export class App {}

for styles: we can use the spread operator for now. I made a pull-request to spread all the collections angular/angular#6049

@NathanWalker
Copy link
Contributor Author

@gdi2290 +1 nice :)

@jkuri
Copy link
Contributor

jkuri commented Jan 7, 2016

I got something working. See #135
Based on @NathanWalker test lib https://github.com/NathanWalker/ng2-cli-test-lib

@jkuri
Copy link
Contributor

jkuri commented Jan 9, 2016

Is it possible to add 3rd party library without adding mappings to system config?

@NathanWalker
Copy link
Contributor Author

With @jkuri 's help, we were able to modify ng2-cli-test-lib to be compatible with SystemJS specifically, and therefore angular cli.
This makes the lib work with the incredible effort @jkuri put forth here: #135

@jkuri
Copy link
Contributor

jkuri commented Jan 10, 2016

Please check out my last commit. Any suggestions/tests/fixes/comments are very welcome. If someone interested to check it out, here is usage example

git clone https://github.com/jkuri/angular-cli -b 3rd-party-libs
cd angular-cli
npm i
[sudo] npm link

then create new project using ng new somewhere

ng new project
cd project
npm link angular-cli
ng libs install ng2-cli-test-lib
ng libs uninstall ng2-cli-test-lib

I tested out and the project builds properly, with injecting with custom or automatic injection or uninstalling the lib afterwards (cleans properly).

If this will hopefully be accepted by maintainers we can define how the library must be created to be compatible with ng libs command and rework the code afterwards. Now ng2-cli-test-lib library uses systemjs-builder to generate SystemJS compatible format and the script looks like this.
If we decided to also support other loaders than system, to facilitate the work of developers of the libs, this script can easily be tweaked to export other formats.

@robwormald
Copy link
Contributor

The more I think about this idea, the less I like it, at least in regards to automatic code generation and injection and the like. I think it's an overreach for what the CLI should do.

@NathanWalker
Copy link
Contributor Author

I can certainly understand that.

To me, cli tools in general are merely a toolbox containing commands that
make tasks easier. Some use more commands all the time whereas to others
maybe never.

The goal here was to simply offer an additional helpful aid to those that
find it helpful.

Personally, this is the type of tool in the toolbox that I would use for
rapid prototyping where a boss comes to me and says, "I'd like to see
what's possible for this idea by tomorrow." I may not use it all the time
but there are times where I would appreciate it.

I think most advanced users (pros) always have a sour taste for tools that
offer to sugar up an otherwise easy task for them. Git GUI apps vs. command
line: perfect example of this.

In the end, it's just about helpful options to me. Ya like honey on your
toast? Great. And for you, grape jam? Fantastic.

On Tue, Jan 12, 2016 at 1:44 AM Rob Wormald [email protected]
wrote:

The more I think about this idea, the less I like it, at least in regards
to automatic code generation and injection and the like. I think it's an
overreach for what the CLI should do.


Reply to this email directly or view it on GitHub
#96 (comment).

@jkuri
Copy link
Contributor

jkuri commented Jan 12, 2016

@robwormald you can simply answer "no" to the first question after installing a lib with that command/task.

@NathanWalker
Copy link
Contributor Author

This has been merged. Documentation is here: #173

@smtx-AliNafees
Copy link

I am using it with angular4 and getting error with ng serve that 'ERROR in ImageLazyLoadModule is not an NgModule'?
any suggestion please?

@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 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants