tonytomov / jqGrid

jQuery grid plugin
www.trirand.com
2.85k stars 1.2k forks source link

jqgrid silently fails when table does not have id #378

Closed gsonnenf closed 10 years ago

gsonnenf commented 12 years ago

This will not work properly. If I add an id it does work. jqgrid should probably either add an id or throw an error saying it needs an id.

justinethier commented 12 years ago

I did a quick test where .jqgrid resolved to a single grid on the page, and the grid loaded just fine. For what it's worth, this test used an older version of the grid (jqGrid 4.0.0). I suspect the grid might run into problems if the class resolves to multiple DOM elements, but have not tested.

Anyway, it might be helpful if you could explain how the grid is not working properly. It would probably be even better if you could post some example code or explain the use case better (such as how many DOM objects the class resolves to, etc).

tonytomov commented 12 years ago

Hello,

Did you have read the wiki doks? A good example is here: http://www.trirand.com/jqgridwiki/doku.php?id=wiki:first_grid#explanation_of_code

Regards

gsonnenf commented 12 years ago

I would expect the grid to work without an id or autogenerate an id if it needs one, and one is not present. Documenting a bug in the wiki docs ( unexpected behavior ) does not mean it isn't a bug.

The bug behavior is no data is loaded when making a request to a rest json service. At worst, it should throw an error saying an id is needed and not silently fail. But really it should just "work" and add an auto generated id.

OlegKi commented 12 years ago

The existence of unique id of <table> is the requirement of the current version of jqGrid not a bug. If you examine all other JavaScript libraries (frameworks) you would see that there are no common way to verify the requirements or options of the libraries. There are no common way to report the errors. For example no alert will be displayed if the validation of requirements will be failed. It's probably not good, but it's the current state of development in JavaScript, jQuery and so on.

Moreover you should not forget, that jqGrid is open source plugin which you get for free. So you can for example modify the code if you want. You can send your suggestion of changes to jqGrid as the poll request on github. Additionally you can, like everyone, improve wiki of jqGrid to describe the requirements more clear. You can choose yourself way.

Regards Oleg

gsonnenf commented 12 years ago

There is a standard way of reporting an application breaking error in JS. Its called "throw" and its part of the ECMAscript specification.

I think jqgrid has some great functionality but needs polish in this instance. It took me half an hour to trouble shoot this error. I was refactoring prior code and removed the jqgrid id's because we were generating them from a template. Now I'm generating random ids instead of using classes in a scoped div.

I'm not submitting this to fix a problem for me. I solved my problem. I'm reporting this so a core developer might decide to fix the issue so other people can benefit from the fix. It should only be a couple lines of code.

I'm hoping at least one core developer is concerned with making jqgrid bug free and he will appreciate that people are testing the grid and making useful suggestions. This bug report is for people who understand that silently failing on core functionality is a bug.

meh-uk commented 12 years ago

@gsonnenf, I don't think any of the main developers care about making jqGrid good. That's why there are a bunch of pull requests open that have been ignored. That's also why the plugin has no unit tests.

The only thing that's good about jqGrid is that its free.

What would really be good would be if someone created a fork that fixed some of these problems.

justinethier commented 12 years ago

@meh-cfl

I don't think any of the main developers care about making jqGrid good. That's why there are a bunch of pull requests open that have been ignored.

I am not one of the main developers, but have contributed patches to jqGrid. One thing to keep in mind is that jqGrid is one of the most complex jQuery plugins - Tony has to be careful which patches he accepts, because fixing one piece of code over here could break something else over there.

By the way, I do agree with @gsonnenf in that jqGrid could be polished up in this regard.

That's also why the plugin has no unit tests.

How would you suggest unit testing the jqGrid code, or JavaScript user interface code in general? Perhaps you could write a few to use as a starting point?

What would really be good would be if someone created a fork that fixed some of these problems.

Nothing is stopping you, although it would probably be more productive to find a way to contribute to the main project. For what its worth, Tony just merged several pull requests...

Anyway, sorry to take this ticket further off track. Thanks,

Justin

meh-uk commented 12 years ago

@justinethier, if a project is very complex, then it seems far more essential that it has a comprehensive suite of tests than if it is simple. Even so it seems difficult to understand why pull #354, for example, hasn't been accepted as that isn't exactly going to have any plausible knock on effects.

With regards to testing the project, well jQuery UI is a "JavaScript user interface" project and it has a comprehensive suite of unit tests, so it is hardly impossible to do.

Potentially of course I could write some unit tests for the project, but given that the main developers won't accept any of my existing pulls, or explain why they aren't suitable, why should I bother?

There are plenty of pull requests from other people which have been left in limbo as well.

farrukhsubhani commented 12 years ago

I think this discussion is going in a different direction and I agree that if your table does not have an id then the requirements are not met. The error throwing aspect is a yes.

tonytomov commented 10 years ago

Hello, Let's end this discussion with happy end. Have added check for id