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

Make angular a dependency in bower.json #429

Closed
wants to merge 1 commit into from
Closed

Make angular a dependency in bower.json #429

wants to merge 1 commit into from

Conversation

joekrill
Copy link

This screws up things like wiredep, and it's really supposed to be there anyway.

This screws up things like wiredep, and it's really supposed to be there anyway.
@ceolter
Copy link
Contributor

ceolter commented Sep 10, 2015

but angular isn't actually a dependency of the grid, it's optional.
http://www.angulargrid.com/example-native-javascript-grid/index.php

@S-YOU
Copy link
Contributor

S-YOU commented Sep 10, 2015

and why wiredep should inject angular too? if you want it you should have angular in YOUR bower.json file. also wiredep only inject files specified in ゙main゙property of bower json object.

@scotthovestadt
Copy link
Contributor

I agree that angular does not belong as a dependency in bower.json for this project.

@ceolter ceolter closed this Sep 11, 2015
@joekrill
Copy link
Author

The problem is, if you're trying to build a dependency tree and decide the order in which scripts need to be loaded, you absolutely need Angular to load first. And the whole point of Bower is to manage your front-end scripts! So without specifying Angular as a dependency here, your scripts have no guarantee of loading in the correct order (if that's how you're building your dependency tree -- and that's really what it's there for in the first place!). So your app will blow up because Angular Grid was loaded before Angular was, so it was never registered as a module.

I mean, the component is called Angular Grid, after all. The title of the site is "A Data Grid for AngularJS". The description of the project on Github is "An Advanced Datagrid for AngularJS". Clearly the target audience here is people who use this with Angular, and that fact that it just so happens to work without it seems to be more of an afterthought. So I don't quite get why it is marketed this way, but no one wants to support it in this way. But, hey, whatever.

Wiredep in particular allows a workaround for this -- so that's what I use. But it's just that -- a workaround -- the correct solution is to set Angular as a dependency in the bower.json. At the very least this should maybe be noted in the documentation somewhere that you need to add this to your bower.json if you're actually using Angular Grid with Angular (and want to use wiredep):

"overrides": {
  "ag-grid": {
    "dependencies": {
      "angular": "*"
    }
  }
}

@S-YOU Adding Angular as a dependency in my bower.json doesn't solve this problem, because there's no way to specify in my bower.json that Angular should be loaded first (i.e. it is a dependency of Angular Grid!).

@S-YOU
Copy link
Contributor

S-YOU commented Sep 11, 2015

@joekrill , I see, fair enough. It make sense about wiredep's injecting order. ag-grid comes before angular. I normally use cdn for angular (even in dev), exclude angular from wiredep, so personally never had problem.

@ceolter
Copy link
Contributor

ceolter commented Sep 11, 2015

fair point re managing the dependencies, and the order of dependencies.

the name, btw, Angular Grid, was because it was targeted at AngularJS, however I don't think AngularJS is suited to building a performant grid. so my stance is the grid should work 'with' angular, not be written 'in' angular. i'm not trying to argue this point, just letting your know how it came about and why the name. given it is able to work without Angular, then I shouldn't block this option off.

there must be other ways to solve this. what do other projects do? eg AngularJS itself will use JQuery if it finds it, otherwise it won't. how is this done? or any other suggestions on how to handle this?

@S-YOU
Copy link
Contributor

S-YOU commented Sep 11, 2015

looks like angular put jquery in devDependencies.
https://github.com/angular/angular.js/blob/master/bower.json

Not sure it is working as expected for wiredep. but it do read devDependencies.
https://github.com/taptapship/wiredep/blob/6fb88e0160274babba15a969b398a64d27750bda/lib/detect-dependencies.js

Edit: noticed that angular have separate repo just for bower, which does not include any deps, but that's a lot more work.

@ceolter
Copy link
Contributor

ceolter commented Sep 11, 2015

devDependencies different story. actually maybe that's not a good example, i'm not sure Angular needs JQuery for it's initialization, in which case, the order wouldn't matter.

@joekrill
Copy link
Author

Using devDependencies won't work -- and doesn't really fit the use case. There was a proposal for optional dependencies in Bower, but that was shot down for some reason.

@ceolter
Copy link
Contributor

ceolter commented Sep 14, 2015

now that i have introduced angular 2 into the mix, it emphasis my point more. to list the dependencies, you would find ag-Grid uses Angular 1 and Angular 2 now - and you definitely don't want both of them included.

@S-YOU
Copy link
Contributor

S-YOU commented Sep 24, 2015

just idea, may be create ag-grid-angular1 and ag-grid-angular2 repositories with only built files and appropriate bower.json and submitting to bower could solve all those problems. (and updating docs to use bower install ag-grid-angular1 or bower install ag-grid-angular2 or for without angular bower install ag-grid)

But you need extra task to commit those repositiories whenever there is a new release.

@ceolter
Copy link
Contributor

ceolter commented Sep 24, 2015

i might have to. i might need 4 flavours though, to cover everything (plain javascript and also web components), and possibly two for angular 2 (sfx vs ecma6 module loading)

@kumarharsh
Copy link

Whoa! Isn't that a LOT of different repos to maintain, for very little gain, and more confusion.
I haven't used wiredep, but reading through it, there is an option to override package config. For me (from a developer standpoint) it'd be a little more work, but overall not too much hassle to do this instead of figuring out which repo to use.

@ceolter
Copy link
Contributor

ceolter commented Sep 24, 2015

may be create ag-grid-angular1 and ag-grid-angular2 repositories

@kumarharsh +1

actually i didn't read this properly, i didn't see the extra repositories needed. yea this not only a lot of work for me, but also makes the grid very confusing.

@S-YOU
Copy link
Contributor

S-YOU commented Sep 24, 2015

@kumarharsh if you don't use wiredep or know what you are doing, you can just bower install ag-grid, its just fine.

@S-YOU
Copy link
Contributor

S-YOU commented Sep 24, 2015

but also makes the grid very confusing.

true, angular have bunch of bower only repos - https://github.com/angular?utf8=%E2%9C%93&query=bower

@ceolter
Copy link
Contributor

ceolter commented Sep 24, 2015

angular also have a team of people to manage it all :)

btw - anyone checking out v2 of ag-Grid?

@S-YOU
Copy link
Contributor

S-YOU commented Sep 24, 2015

btw - anyone checking out v2 of ag-Grid?

still reading migrating guide, will report once I done.

Only one curious, do you really need to name function with Func in the name like in agGridGlobalFunc? may be there is some story behind.

@ceolter
Copy link
Contributor

ceolter commented Sep 24, 2015

no interesting story.

the added piece is 'GlobalFunc', not just func.

@kumarharsh
Copy link

@S-YOU I'm not using wiredep, but from my experience with requirejs, browserify and webpack, I've understood that not all repos follow the same convention. Sometimes it's just due to lack of resources on the dev's behalf, but also sometimes because the structure of the repo can not be changed just to accommodate another build tool, (see Modernizr for instance, which needs a specific build/download step). So, the good build tools have some sort of escape hatch in the form of shims or something like that to support them, and we're actively encouraged to use the shims.

@S-YOU
Copy link
Contributor

S-YOU commented Sep 24, 2015

@ceolter just reporting, I have successfully upgrade to v2 of ag-grid, with one fix on rowClass that I sent PR. all others features that I used forPrint, setRowData, awk to ag works perfectly. Thanks for the awesome grid as ever.

@ceolter
Copy link
Contributor

ceolter commented Sep 24, 2015

@S-YOU great, thanks for the feeback. i've just accepted your PR and will do another release.

i hope the new interface makes sense. there is a bit more to it, but it should make the grid easier to understand (rather than just a bunch of stuff in gridOptiosn), and also get us all thinking towards ng2 and web components.

@S-YOU
Copy link
Contributor

S-YOU commented Sep 24, 2015

yeah, setRowData make much sense, and also better for multiple grid I think. thanks for quick response.

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

Successfully merging this pull request may close these issues.

None yet

5 participants