pinia-orm icon indicating copy to clipboard operation
pinia-orm copied to clipboard

trying out the new axios plugin

Open vesper8 opened this issue 1 year ago • 13 comments

Environment

No response

Reproduction


Describe the bug

I'm trying out the newly released Axios plugin and am coming across some issues. First of all, thanks so much for making it, as a result I can finally begin to migrate from vuex-orm and it couldn't have come at a better time because I was reaching the limits of what vuex-orm is capable of when I started needing to retrieve and update deep pivot fields.

Anyway, first of all, I found a typo in the setup guide. You refer to createPiniaOrmPluginAxios but it really should be createPiniaOrmAxios

You also write form instead of from.

And you keep referring to useAxiosRepo but without ever mentioning import { useAxiosRepo } from '@pinia-orm/axios';

I know it may be obvious but I also think the setup guide should assume the reader might be completely new to pinia-orm

Anyway on to the real issue.

When I attempt do to this:

    await useAxiosRepo(User).api().get('/api/v1/users', {
      persistBy: 'insert',
    });

I get this error Uncaught (in promise) Error: [Pinia ORM Axios] The axios instance is not registered. Please register the axios instance to the repository.

However, if I add this line above

console.log(`BEFORE: useRepo(User).all().length: ${useRepo(User).all().length}`);

Then I don't get the error. Basically just making a simple call, any call, using useRepo seems to make the error go away.. might be some kind of race condition? Hopefully you can make sense of what the issue is here.

That's it for now.. I will continue my migration and report back my findings.

Unrelated but I was surprised to see that .count() is not a valid method. With vuex-orm I would often get the count like this User.query().count(). This method is very prevalent in Laravel, which I also use extensively. Would be a nice little helper to add I guess. But as you can see above I figured out you can simply chain .length after the .get() too.

Also,

I use

{
      persistBy: 'create',
}

and

{
      persistBy: 'insertOrUpdate',
}

a lot and was surprised to see those are not valid values with pinia-orm. I couldn't really see where the difference is explained between save and insert in pinia-orm's persistBy configuration.. but maybe I just missed it in the docs.

I guess there isn't an official, or unoffocial migration guide anywhere yet (from vuex-orm) ?

Thanks a lot for making and further developing pinia-orm!!! I look forward to using it now that the Axios plugin is in!

Additional context

No response

Logs

No response

vesper8 avatar Sep 19 '23 20:09 vesper8

Hrmm.. perhaps the biggest issue I'm now facing is that I'm not sure if your axios plugin supports nested relationships?

With vuex-orm's axios plugin, I call a single api and it returns a huge object with dozens of nested relationships, that single api call ends up filling up thousands of entities across dozens of models and their nested relationships.

At first glance it initially appears that this does not work with your axios plugin... ? Please tell me that's not so and I'm just misusing it.. ?

vesper8 avatar Sep 19 '23 21:09 vesper8

Hey @vesper8 , thanks for the input. You are abosultly right. I rushed a bit the axios documentation because i wanted to release it. I am going to update it in the next days. Also i try to put a migration guid. You already come accross one the differences. In pinia-orm/axios the options for 'persistBy' are different. There is no longer a 'insertOrUpdate'. Right now there are only 'save' and 'insert'. Just use save as default.

The other point is that your missing something. yes. It should save all the nested relations and so on.

The best examples how it works are the unit tests right now 😄

I am going to start an PR for this and maybe you can help me out completing the migration guide because your case is the best secenario to catch all stuff.

CodeDredd avatar Sep 20 '23 06:09 CodeDredd

Thank you for gettig back to me @CodeDredd

I managed to get the relationships to load now, it was because of my persistBy value. insert leads to no relationships being loaded.. and save works. Does this sound normal to you?

Also, I think I may have found a bug.. it has to do with boolean fields. I get thousands of warnings in my console [Pinia ORM] Field myModel:isSystemView - 1 is not a boolean

In my api request, I'm just returning these as integer 1 or 0. This should work shouldn't it?

I was able to make the warning go away by changing the return value like so: 'isSystemView' => $architectureView->is_system_view ? true : false,

But according to your docs, it should have worked with 1s and 0s (and it worked before with vuex-orm).

Lastly, what about the issue I mentioned above regarding Uncaught (in promise) Error: [Pinia ORM Axios] The axios instance is not registered. Please register the axios instance to the repository. when I

    await useAxiosRepo(User).api().get('/api/v1/users', {
      persistBy: 'insert',
    });

I'm still having this issue if I don't first "initialize it" by making an arbitrary call using useRepo.

vesper8 avatar Sep 20 '23 07:09 vesper8

I have same issue with unregistered axios instance. However, I'm using custom repository:

export default class MessageRepository extends AxiosRepository<Message> {
    use = Message;

    fetch = async (roomId: string) => {
        return await this.api().get(`/rooms/${roomId}/messages`);
    };
}

const messageRepo = useRepo(MessageRepository);

I can't use useAxiosRepo because its signature doesn't fit (it only accepts model class). Is it ok to use useRepo instead?

Also, in the docs I've found another typo - pina instead of pinia in some places.

daniser avatar Sep 20 '23 08:09 daniser

@vesper8 Help is on the way. I try to start 1.8.0 which address this. It's a think which i already started months ago: #790 ... in that PR insert also can handle relations. I will add this without breaking changes because that was the reason why i didn't merge it early.

@daniser I regret i did't put an example how to do this in the docs, because i thought that some1 surely want to use useRepo ^^. But in my thinking it was better because not every model has an api, but if it is something you want, well here is the code. You need to define your own plugin. I will add it to the package for 1.8.0.

I will also adapt the signature of useAxiosRepo so it also accepts Repositories

plugins/piniaOrmAxiosPlugin.ts

import { PiniaOrmPlugin, definePiniaOrmPlugin } from 'pinia-orm'
import type { GlobalConfig, Config } from '@pinia-orm/axios'
import { useAxiosApi } from '@pinia-orm/axios'
import axios from 'axios'

export function createPiniaOrmAxios (axiosConfig?: GlobalConfig): PiniaOrmPlugin {
  return definePiniaOrmPlugin((context) => {
    context.config.axiosApi = axiosConfig
    context.repository.axios = axios 
    context.repository.globalApiConfig = axiosConfig
    context.repository.apiConfig = {}
    context.repository.api = () => useAxiosApi(context.repository)

    return context
  })
}

And if you use typescript also add this to your own types/pinia-orm.ts:

import type { AxiosInstance } from 'axios'
import type { Request, GlobalConfig, Config } from '@pinia-orm/axios'

declare module 'pinia-orm' {
  namespace 'Repository' {
    /**
     * The api client.
     */
    export let axios: AxiosInstance | null

    /**
     * The global api configuration for all models.
     */
    export let globalApiConfig: GlobalConfig

    /**
     * The api configuration for the model.
     */
    export let apiConfig: Config

    /**
     * Set the given axios instance.
     */
    export function setAxios(axios: AxiosInstance): void

    /**
     * Get the api instance.
     */
    export function api(): Request
  }
}

CodeDredd avatar Sep 20 '23 11:09 CodeDredd

Thanks, it worked!

Strangely, this part wasn't needed (probably due to generic signature of useRepo?):

And if you use typescript also add this to your own types/pinia-orm.ts:

import type { AxiosInstance } from 'axios'
import type { Request, GlobalConfig, Config } from '@pinia-orm/axios'

declare module 'pinia-orm' {
  namespace 'Repository' {
    /**
     * The api client.
     */
    export let axios: AxiosInstance | null

    /**
     * The global api configuration for all models.
     */
    export let globalApiConfig: GlobalConfig

    /**
     * The api configuration for the model.
     */
    export let apiConfig: Config

    /**
     * Set the given axios instance.
     */
    export function setAxios(axios: AxiosInstance): void

    /**
     * Get the api instance.
     */
    export function api(): Request
  }
}

So maybe useAxiosRepo should NOT accept Repositories, just use "regular" useRepo on repositories that extend AxiosRepository? I dunno what looks more logical.

daniser avatar Sep 20 '23 14:09 daniser

@CodeDredd that's great that you're working on 1.8.0.

I was indeed wondering how come there is no update method. I hope you will explain how your new insert method will differ from the current save method.

I still have the Uncaught (in promise) Error: [Pinia ORM Axios] The axios instance is not registered. Please register the axios instance to the repository.

I'm getting around it by simply adding this line at the top of my pinia axios request:

    // hack to fix issue with pinia-orm 1.7.2 (Uncaught (in promise) Error: [Pinia ORM Axios] The axios instance is not registered. Please register the axios instance to the repository.)
    useRepo(User).all();

vesper8 avatar Sep 20 '23 14:09 vesper8

I am also getting the The axios instance is not registered. Please register the axios instance to the repository error.

ocratravis avatar Sep 26 '23 18:09 ocratravis

How is the Progress on 1.8.0? Any News? It has been a month.

telion2 avatar Nov 03 '23 08:11 telion2

@CodeDredd Hello : )

Following up on this issue. I found that if I make a request to the API using useAxiosRepo and the response returns an empty nested relation, then it will not overwrite the content of the ORM.

For example if I'm toggling whether a user is in a team or not, if I make a request to the team api:


        await useAxiosRepo(Team)
          .api() //
          .post(
            `/api/teams/${teamId}`,
            {
              userId: userId,
            },
          );

This API returns the team with all nested relations. If the team did not have users and I've now added a user, then it correctly saves the new user. However if I am removing a user from the team and the API returns an empty array for the team->users, then it will not remove the existing users, I have to go and remove them from the pivot table manually. I would expect that there would be a persistBy option that I could pass that would result in the the team's current users being overwritten by whatever the API returns, in this case, an empty array.

Am I making sense? Thanks!

vesper8 avatar Jan 19 '24 16:01 vesper8

@CodeDredd Any update on this? This persistBy missing an option to overwrite the ORM is driving me crazy.. it's really bad.

For example if I use the Axios plugin to populate a Post with 5 comments and then if I make another request to update the post and now the API is returning the post with 4 comments, it does update the post and 4 comments in the ORM but it does not remove the post that is now missing.

This Axios request that I'm making is not necessarily a delete request, maybe a user has deleted their comment in another instance in the meantime and that's why it is now only returning 4 instead of 5.

The point is, I want to be able to tell pinia-orm to overwrite the nested relations instead of the current behaviour which updates/appends.

Vuex had this setting in the form of 'persistBy: insert'. A bit misleading but this used the response from the API as a source of truth. If the Post previously had 10 comments and now we're getting a Post response that only has 4 comments, then we overwrite all of that post's comments and now there's only 4.

Pinia-ORM does not make it possible to do this right now and it's really bad. I am forced to manually delete instead of just relying on the api's response as the truth.

Would really really like to see this fixed via a new 'persistBy' option.

Many thanks

vesper8 avatar Feb 01 '24 12:02 vesper8

@vesper8 I will soon handle your problem. Trying to work out issues from the bottom in the list. But giving your a bit more prio.

CodeDredd avatar May 02 '24 13:05 CodeDredd

My solution to the error "Error: [Pinia ORM Axios] The axios instance is not registered. Please register the axios instance to the repository." is to pass axios directly to the repository, since I also couldn't get it to work during initialization of the plugin.

const deviceRepostiory = useAxiosRepo(DeviceEntity);
deviceRepostiory.setAxios(axios);

Thank you for your work @CodeDredd. I'm looking forward to the next release.

andywcoder avatar May 23 '24 11:05 andywcoder