twitchtv / node-apicalypse

The apicalypse query language client for nodejs
MIT License
81 stars 11 forks source link

Query Builder is not Immutable #4

Closed dacioromero closed 5 years ago

dacioromero commented 5 years ago

I normally set up Apicalypse with parameters like this for use with the IGDB v3 API:

const IGDB = apicalypse({
    baseURL: "https://endpoint-alpha.igdb.com",
    headers: {
        'Accept': 'application/json',
        'user-key': process.env.IGDB_API_KEY
    },
    responseType: 'json'
})

An issue arises when building multiple requests off of that because the query builder methods aren't immutable. This results in new queries to "inherit" previous calls built off of IGDB messing up the results of the request.

Temporarily we can create a function that returns our desired default values:

const IGDB = () => {
    return apicalypse({
        baseURL: "https://endpoint-alpha.igdb.com",
        headers: {
            'Accept': 'application/json',
            'user-key': process.env.IGDB_API_KEY
        },
        responseType: 'json'
    });
}

In the long-term I think it would be benficial for the query builder methods to be immutable.

Perhaps something similar to pypika's solution (although a different language) of a decorator could be used?

krazyjakee commented 5 years ago

Good catch! How could we change the return value of this function to achieve immutability?

https://github.com/igdb/node-apicalypse/blob/master/src/index.js#L103

krazyjakee commented 5 years ago

Scrap that. axios.create gives a new instance, so this should fix it.

https://github.com/igdb/node-apicalypse/blob/dev/src/index.js#L88

@DacioRomero can you test this in your use case?

dacioromero commented 5 years ago

Sorry for the massive delay in communication.

I updated my server to use the released versions of APICalypse and IGDB v3 API.

I'll update you in a week or so if that helped.

krazyjakee commented 5 years ago

@DacioRomero I ran into the same bug myself and can confirm the change worked. Thank you very much for reporting this! I'm gonna cut a new version now.

krazyjakee commented 5 years ago

some more info https://github.com/axios/axios/issues/1522

krazyjakee commented 5 years ago

Found other cases https://github.com/axios/axios/issues/1383

dacioromero commented 5 years ago

After looking over your responses I think there was a misunderstanding.

The issue I was having was that the query builder methods aren't immutable, so I reuse the first APICalypse object in the first code snippet for all of my requests previous requests effect how future ones behave.

I think that .fields(), .sort(), etc. should return new APICalypse object rather than this which would allow users to have a base for future requests to be built off of.

krazyjakee commented 5 years ago

I pushed a new version today which correctly resets the object when using the same instance twice.

The issue with returning a new apicalypse instance for fields and sort is that you wouldn't be able to chain the methods as one would have no memory of the other.

krazyjakee commented 5 years ago

Thanks to a discord user I have been able to reproduce the issue.

const IGDB = apicalypse({
    baseURL: "https://api-v3.igdb.com",
    headers: {
        'Accept': 'application/json',
        'user-key': userKey
    },
    responseType: 'json'
});

then repeatedly using it

const response1 = await IGDB
        .limit(50)
        .request('/games');

const response2 = IGDB
        .offset(50)
        .request('/games')

would smash the first and second requests together. I have now cut a release with apicalypse@0.1.3 that resets the query when firing the request function. The fix will allow you to use the library this way.

Sorry it took so long to figure out.

Let me know if this addresses the issue.