ratiw / vuetable-2

data table simplify! -- datatable component for Vue 2.x. See documentation at
https://vuetable.com
MIT License
2.16k stars 400 forks source link

Vue Resource should not be imported within Vuetable.vue #55

Open brandonferens opened 7 years ago

brandonferens commented 7 years ago

In my opinion, requiring the use of vue-resource should be up to the developer. I am already including vue-resource and as such, the error Cannot redefine property: $url gets thrown.

Please remove the import and force developers to pull it in themselves as a dependency.

Unless of course there is a specific reason for this. I would also like to see the ability to change out vue-resource for another similar package like axios.

felorhik commented 7 years ago

I would have to say adding this as a feature would probably make this plugin more usable. I use vue-resource so its currently not much of a problem for me (but planning to swap to axios in the future).

I can see 2 ways to add this.

  1. Create a callback function, if none is set it imports and uses vue-resource by default, the parameters passed can be the same from loadData only adding apiUrl and httpOptions. It can default to the style of vue-resource if the package in question needs something different then I can see needing to transform it on the devs end. This way I would say is more tedious (since its needed on EVERY table) so I suggest suggestion 2

  2. Add global settings. This means making Vue.use(VueTableConfig) or something similar. It would add the settings into the global vue instance, this would allow a similar callback to suggestion 1 but global, and other settings can be made global (like css to better integrate with other css frameworks). It can also be made not required, using defaults if its not set. This is a similar idea to setting global headers for whatever ajax lib you might use.

You do not seem to integrate deeply with vue-resource but I can understand why it might be challenging to allow swapping.

I am willing to work on a PR when I got time, but I can see this leading to a lot of refactoring, too much work to do before asking your opinion first @ratiw Also you may want to do it differently then i might. I can see this being possible without breaking anything or needing any migration.

What do you think?

This would solve #43 and #35

brandonferens commented 7 years ago

@jordanramstad All of that seems fine. The issue I ultimately see is that the plugin is importing Vue and Vue Resource itself. Which doesn't make any sense as a .vue file is referencing those. Those resources shouldn't be called from within the component. I fixed this by literally commenting out lines 87-89 in Vuetable.vue.

felorhik commented 7 years ago

@brandonferens I guess your right on that. I seen this issue and thought to comment in the most recent one and reference the others.

However without doing that import if you do not use vue-resource it would break, the problem is that if you use something different, you might not want the overhead of including another ajax library when you primarily use a different one.

Laravel now uses axios by default and vue-resource has been dropped as the "official" package. https://medium.com/the-vue-point/retiring-vue-resource-871a82880af4#.y2ko8usbt

brandonferens commented 7 years ago

Yeah, I use Laravel and would prefer to use axios. Perhaps a wrapper for axios could be built that has the same interface as vue-resource.

ratiw commented 7 years ago

@jordanramstad @jordanramstad Thanks for the comments and dicussions. :)

First, I don't have anything against vue-resource nor axios. They're just libraries that I make use of as they're both serving the purpose of not having to re-inventing the wheel.

Replacing vue-resource with axios is really not so hard and I am already planning to do it. The reasons that it is still there (even though it's not that hard) are because

@brandonferens To be clear, I think your issue is not really with the vue-resource itself, but rather "re-importing" the library. I mean if I use axios instead of vue-resource but I'm still importing axios like I did in Vuetable.vue, it would still cause the problem, right?

If so, that's the problem I would like to fix and your input would help a lot. But if I do not importing it inside .vue file, I would not feel like it is using it internally. But I'm totally open to discussion on this.

Because I probably write code different than yours so I've never experience that issue. My guess (I might be wrong here) is that you write the client-side code as Single Page Application (SPA), so when the bundler tries to compile the code, it sees the redeclaration of vue-resource.

I myself do not write SPA because the tooling was quite confusing (before vue.js comes along). And with Laravel and Vue.js I find myself more comfortable not using the SPA approach.

@jordanramstad I (personally) think that once I replace vue-resource with axios, many more people would be happy to hear that. But I still don't think making it swap-able is the way to go. If you would like to help, making the PR that replace vue-resource with axios would be just fine and it would be really apprecaited.

In closing, would this still be an issue if I decided to use Promise instead of using any other AJAX library to do the work for Vuetable? Please don't get me wrong. I really love this kind of discussion. It really opens my mind and the way I approach the code.

Cheers,

felorhik commented 7 years ago

@ratiw What do you mean by replace with Promises? Both axios and vue-resource use them.

I am still a bit unsure of doing just a replace, it seems to me like it would just replace the problem with another one.

Here is an extension of what I mean by config:

Vue.use(new VueTableConfig({
  css: {
    table: {
      tableClass: 'table table-striped table-bordered',
      loadingClass: 'loading',
      ascendingIcon: 'glyphicon glyphicon-chevron-up',
      descendingIcon: 'glyphicon glyphicon-chevron-down',
      sortHandleIcon: 'glyphicon glyphicon-menu-hamburger'
    },
    pagination: {
      wrapperClass: 'pagination pull-right',
      activeClass: 'btn-primary',
      disabledClass: 'disabled',
      pageClass: 'btn btn-border',
      linkClass: 'btn btn-border'
    }
  },
  icons: {
    first: '',
    prev: '',
    next: '',
    last: ''
  },
  ajax ({ apiUrl, httpOptions, success, failed }) {
    axios.get(apiUrl, httpOptions)
      .then(success)
      .catch(failed);
    // or success/failed can be removed and instead return it as a promise
  }
})

Then you can make a variable like this.$vueTableConfig would be available everywhere in vue, containing the configuration. I understand this is quite extensive though. This would make future updates have a clear place to adjust how vuetable handles different situations, without having to copy that configuration into every parent of every table. This could leave the settings in each table left for special styles / configurations, using the root config as the default.

Sorry for the massive comment, like you I do love discussions like this :).

ratiw commented 7 years ago

@jordanramstad What I try to say was that if I do not use any ajax library like axios or vue-resource, and use the javascript's Promise instead, people wouldn't have asked me to change from vue-resource to axios or anything else.

Your example of using VueTableConfig object looks nice. But I wonder how it would be different than just import/require a normal javascript object and assign it to the css prop, except that it has the ajax callback (which I still don't think it's unnecessary).

  <vuetable ref="vuetable"
    :css="css.table"
  ></vuetable>
  const css = {
    table: {
      tableClass: 'table table-striped table-bordered',
      loadingClass: 'loading',
      ascendingIcon: 'glyphicon glyphicon-chevron-up',
      descendingIcon: 'glyphicon glyphicon-chevron-down',
      sortHandleIcon: 'glyphicon glyphicon-menu-hamburger'
    },
    pagination: {
      wrapperClass: 'pagination pull-right',
      activeClass: 'btn-primary',
      disabledClass: 'disabled',
      pageClass: 'btn btn-border',
      linkClass: 'btn btn-border'
    }
  }
brandonferens commented 7 years ago

Just to be clear, the idea of using axios was a second thought. The original idea behind this issue was the re-importing of both vue and vue-resource. So it might be helpful to pull the axios vs. vue-resource vs. another ajax library into a different thread.

@ratiw Yes, you are mostly right about my situation. I don't have a SPA, but I do have a single app.js file that includes the different libraries I use throughout the site. I also understand how you are attempting to use it. My suggestion would be that a check is run. If Vue or VueResource exist, then just move on, otherwise load them.

felorhik commented 7 years ago

@brandonferens I understand I did somewhat hijack this issue ><. My idea does solve it but its far more complicated of an answer then this needs.

@ratiw I understand your point of view on it. I am going to stop here for now to avoid the original issue getting to sidetracked (importing vue-resource regardless of if it is loaded or not). I may make a pull request and use that to continue this later (not trying to tell you how it needs to be, just really like this package and want to see it get all the stars :D)

ratiw commented 7 years ago

Many thanks @jordanramstad. Looking forward to your input very soon.

@brandonferens I've just created a sample project using Vuetable-2 in Laravel 5.4, but I don't seem to find the same problem as you. Can you please check it out and let me know how it's different in your situation.

brandonferens commented 7 years ago

Sorry, I haven't gotten back to you sooner. Flooded with Browser testing issues. I'll respond tomorrow hopefully.

lmj0011 commented 7 years ago

@brandonferens

I fixed this by literally commenting out lines 87-89 in Vuetable.vue.

I'm going to try this

I'm having the same issue in my adonuxt.js app error Cannot redefine property: $url

made an issue in the adonuxt repo, but it looks to belong here.

ratiw commented 7 years ago

@lmj0011 I'm still not sure what is the root cause of the problem. But I haven't defined any variable that named $url in Vuetable.vue code. So, I doubt that it was caused by Vuetable.

I also create a sample project and re-import vue-resource and Vue.use() it and still did not get any error as mentioned here.

brandonferens commented 7 years ago

I believe the one thing with the Laravel 5.4 sample project that is different than mine is that it doesn't pull in vue-resource by default any more. The base Laravel install uses axios now as it is a better option. Try requiring vue-resource in resources/assets/js/bootstrap.js and see if you run into the issue.

ratiw commented 7 years ago

@brandonferens Already tried as you suggested and did not see any error.

This is what I did

window.Vue = require('vue');
Vue.use(require('vue-resource'));

also this,

window.Vue = require('vue');
import VueResource from 'vue-resource';
Vue.use(VueResource);

Both were compiled successfully and everything runs with any error in both the terminal and the browser's console.

brandonferens commented 7 years ago

Perhaps the difference is that I am pulling in vue resource with just require('vue-resource');. I'm not using Vue.use();.

lmj0011 commented 7 years ago

instead of using an http lib in vuetable2 https://github.com/ratiw/vuetable-2/blob/master/src/components/Vuetable.vue#L316

making a raw http request using xhr or fetch would avoid conflicts with other libs, I believe

lmj0011 commented 7 years ago

@ratiw

https://github.com/ratiw/vuetable-2/blob/master/src/components/Vuetable.vue#L316 if you could tell me what's in the this.httpOptions object I can try implementing http request without a lib.

ratiw commented 7 years ago

@brandonferens I don't think that's the case. Vue.use() function is just registering vue component with Vue, so that it knows the component existance.

@lmj0011 I'm just trying to understand what would be the problem that causes this issue as I do not encounter it and also do not have enough information on this.

Part of httpOptions (params specifically) is just the object that will later be translated into query string or URL parameters. You can find more info here. Basically, you just need to use the returned value from getAllQueryParams() to be sent as URL parameters.

adifaidz commented 7 years ago

@ratiw I can confirm that using Vue.use(require('vue-resource')) works for me while require('vue-resource'); does not, and I ran into no issues for now. So now my bootstrap.js looks like this

window.Vue = require('vue');
Vue.use(require('vue-resource'));

window.axios = require('axios');

window.axios.defaults.headers.common = {
    'X-CSRF-TOKEN': window.Laravel.csrfToken,
    'X-Requested-With': 'XMLHttpRequest'
};

Hope this helps

lmj0011 commented 7 years ago

@ratiw

Part of httpOptions (params specifically) is just the object that will later be translated into query string or URL parameters. You can find more info here. Basically, you just need to use the returned value from getAllQueryParams() to be sent as URL parameters.

Decided to try to use xhr-request in place of vue-resource. I wasn't able to successfully get all the unit tests to pass

I'm just trying to understand what would be the problem that causes this issue as I do not encounter it and also do not have enough information on this.

I duplicated the error in an adonuxt project:

https://github.com/lmj0011/vuetable2issue

atinux commented 7 years ago

Hi @ratiw

Great job with vuetable-2, to make a point of why axios should replace vue-resource is really simple but also very important: server-side rendering

Actually, the error of $url if due to the server-side rendering.

lmj0011 commented 7 years ago

Has anyone started work on replacing vue-resource with axios. I'm going to give it another shot this weekend, if possible.

lmj0011 commented 7 years ago

using axios PR: https://github.com/ratiw/vuetable-2/issues/65

dvatp commented 7 years ago

@lmj0011 Just confirmed that your changes work.

tsankuanglee commented 7 years ago

Just wanted to add that I had Cannot redefine property: $url thrown because I was using vue-resource-2. Changing it to vue-resource works. It was very hard to debug. Probably another reason to let user choose.

cristijora commented 7 years ago

I still don't see the need of using by default an http library inside vue-table.

  created(){
   if(!this.$http || !this.$http.get ){
     console.warn('Vue table requires a http library based on promises in order to work ');
   }
  }

This way the user can choose whatever library to use, can put global authorization headers or other headers where and when he wants. Vue table will simply execute the requests and thus will not need to use Vue and VueResource or axios internally.

Take this case for example:

  1. VueTable uses VueResource or axios internally and injects $http on each component.
  2. User decides to use another library an also injects $http on each component.
  3. What if the libraries conflict / override functions or properties?
lmj0011 commented 7 years ago

the switch to axios is here: https://github.com/ratiw/vuetable-2/commit/fd91517de6844e2b3822750fee7d2752910ea19d

npm install from the develop branch of this repo if you want it now

lmj0011 commented 7 years ago

This way the user can choose whatever library to use

That would be counter-productive since this adds additional work just to get this library running. There's already enough to take in when learning to use this lib.
The only reason this is an issue is because of JS frameworks trying to incorprate server side rendering. VueResource just isn't/wasn't ready for it. ratiw explained his reasons for not letting the user choose the HTTP lib

naumf commented 7 years ago

This way the user can choose whatever library to use, can put global authorization headers or other headers where and when he wants. Vue table will simply execute the requests and thus will not need to use Vue and VueResource or axios internally.

I agree with @cristijora, someone might have an axios instance with default authorization header, global response interceptor that handles unauthorized access and other global configs. Making the request with this.$http.get will come in handy for these use-cases.

That would be counter-productive since this adds addition work just to get this library running.

@lmj0011 For those that don't have global headers/interceptors and just want to get this library running they can use the lib's axios dependency.

This is my suggestion:

if (this.$http && typeof this.$http.get === 'function') {
    // make the request using this.$http.get
} else {
    // make the request using axios.get
}
ratiw commented 7 years ago

I think I'll have to take this into further consideration in the future release.

The discussions in this thread really have given me an opportunity to see various use cases and possible solutions about the issue beyond my simple expectation.

I would love if anyone can submit PR(s) suggesting the couple of solutions to this issue and have further discussion on it.

Thanks, everyone. :)

maulshh commented 7 years ago

This issue is real and I think @cristijora solution should be implemented,

  1. What if the libraries conflict / override functions or properties?

This will always happen as the developers using token mechanism like basic or bearer for example (laravel-passport authentication) in the default configs of axios or vue-resource.

Do developers have to set those default settings inside vuetable or manually inside every table? The auth problem sometimes also hard to notice while working in the back-end, and to fix it adds layers of complexity to the table code.

I always copy vuetable to my source and almost never used it as a library but it's actually a shame