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

Allow placeholders which are not POSTed as data #13

Open
Crote opened this issue Feb 6, 2019 · 18 comments
Open

Allow placeholders which are not POSTed as data #13

Crote opened this issue Feb 6, 2019 · 18 comments

Comments

@Crote
Copy link

Crote commented Feb 6, 2019

Currently, when requests are made with url placeholders, the placeholder values are always submitted to the server as part of the data. This is in many cases undesirable.

Usually, the server does not actually need those to be posted, as they can be retrieved from the url. In the best case, the server will just ignore them. In the worst case, the request will throw an error because the data contains unknown values. In addition to this, it makes it hard to change this value: for a template /foo/:name, if we want to submit a change to the name value, we can't use this template: the url requires the old value, but the data requires the new value, so we have to use a template like /foo/:_name, and we're once again adding data to our request which the server must ignore.

Ideally, the data and parameters would be two separate parameters, but that would break a lot of existing code. Perhaps the POST-like requests can have an optional second parameter, where it defaults to current behaviour when it is not set?

@davestewart
Copy link
Owner

davestewart commented Feb 9, 2019

Hey Laurens,

You've spotted Axios Action's dirty little secret!

Yes, everything gets submitted because generally it doesn't matter, and it's easier, but I've always suspected that at some point it may become a problem.

And you make a great point about changing a name.

Let's use this thread to think of solutions.

Another idea: a constructor option to remove the URL variable from the data payload.

@davestewart
Copy link
Owner

Actually, seeing as you specifically have this problem, can how do you find preparing the payload in the first place?

Do you feel you're forced into adding values to the payload that weren't there originally, just to satisfy the library's requirement to populate the path?

I'm just trying to think through the problem end to end.

@Crote
Copy link
Author

Crote commented Feb 9, 2019

To be clear, this is not something which is currently a problem for my implementation. I just came across it and it seemed like something which could be a problem in the future.

The reason I noticed it, is because I tried to do something like posts.update(id, {title: 'new post', body: 'this is updated text'}), because it wasn't clearly documented how submitting data and placeholders interacted and that seemed the obvious way to do it considering the rest of the api. In my reasoning, it's not uncommon for the data and the url definition to come from completely different parts of your application, so putting them in one object would require merging them together when calling the api, so that doesn't really make sense.

Fortunately I'm using Ruby on Rails as backend, which happily ignores any parameter it does not recognize. so for me it's not a big deal at the moment.

However, consider something like the Gitlab Pages API, which has a post parameter which overlaps with a url placeholder. How would one implement that? Currently, the only option is to rename your placeholders client-side and hope the server is willing to ignore it.

Removing the placeholder before submitting would work if your server is very strict on the data it accepts, but it would still require the user to use a different name for their parameter. Note that this could also cause hard-to-find bugs if at some point in time the data submitted to the server gains an additional field, which just happens to overlap with an existing parameter.

Ideally, it should be impossible for the parameter name to overlap with a data key, but I'm not sure if that's even possible to do.

Perhaps defining symbols for the placeholders as properties on the api function would work, giving something like

Object.defineProperty(posts.update, 'id', { value: Symbol('id'), writable: false })
----
posts.update({posts.update.id: id, title: 'new post', body: 'this is updated text'})

It's not very elegant, but you can be sure that those don't overlap any payload data, so you can first look for those to fill in the parameters, falling back to current behaviour if they're not found.
Unfortunately, the names can also overlap existing properties, so we get something like

posts.update({posts.update.param.id: id, title: 'new post', body: 'this is updated text'})

which is just way too verbose in my opinion.

@davestewart
Copy link
Owner

because it wasn't clearly documented how submitting data and placeholders interacted

Hmm. I should certainly make that clear if it isn't already.

OK, if this isn't a priority then I'll have a little think over the next short while and we can pick this up as and when

@rlweb
Copy link

rlweb commented May 22, 2019

Yep, we've just hit this same problem and due to the way we are using our APIDoc as server side validation we need the user ID removed from a call which would be like

const api = new APIResource(axios, './user/:userId/consent');
data = api.read(1); // returns {photo:true,public:false}
data.public = true;
api.update(data);

Would it not be possible to use the second param be used, such as?

const api = new APIResource(axios, './user/:userId/consent');
data = api.read(1); // returns {photo:true,public:false}
data.public = true;
api.update(data, {userId:1});

@davestewart
Copy link
Owner

Hey Rhys,

OK, looks like I'll have to do something about this sooner rather than later then!

So that took me a couple of reads, but you're saying that the return payload no longer has a parameter that will populate :userId so your idea is to pass additional path parameters as a second parameter.

It's been a while since I read this post, but I like it on the surface of things.

Is there no way you would do something like this, even with a helper?

api.update({...data, public: true, userId: 1});

@rlweb
Copy link

rlweb commented May 22, 2019

Hi Dave,

The issue is that our API has been built to ensure there is not additional unused parameters? So such as it see's userId and returns a validation exception!

@rlweb
Copy link

rlweb commented May 22, 2019

Hi @davestewart , I've attached a PR although its a little bit awkward with the setup of the id say within the read call. I've essentially allowed to be the last replacement during the replaceTokens function.

@davestewart
Copy link
Owner

Thanks for the PR.

Not sure if I'll accept it right off the bat as the only other outstanding issue is #14 which is for exactly the same issue.

There's a discussion in there.

Did you, or can you, take a look and feed back?

@stephandesouza
Copy link

Hey guys, "new project, new challenges" here.

@davestewart your plugin fits well on our need, but we got the same problem from @rlweb, and his fork worked like a charm.

@davestewart
Copy link
Owner

Hey hey, sorry for the lack of progress on this project - I have been mega-busy, but am now free again.

Can you pull from that fork in the meantime, and I will take a look in the next few days?

@stephandesouza
Copy link

Of course, if you need any hands for testing purposes, I'm here to help.

@vizio360
Copy link

vizio360 commented Jan 4, 2020

Hello, spotted the same issue here as well, specifically for a child resource scenario.

I have a rest/groups/:groupId/members end point which you can use to add new members to a group by issuing a POST request.
As per how axios-actions combines url parameters with body data, I end up sending the :groupId url param in the body of the POST request.

Any idea when this will be addressed?

@stephandesouza
Copy link

Dont forget us @davestewart :)

@AlMuz
Copy link

AlMuz commented Dec 21, 2021

@davestewart Hello! People are still waiting!

@davestewart
Copy link
Owner

I had forgotten about this but... Christmas time is Open Source time! Maybe a 🎁 coming, that being the case...

@davestewart
Copy link
Owner

I haven't forgotten about you all! I'm fulfilling all my Open Source responsibilities by the end of Jan!

@davestewart
Copy link
Owner

davestewart commented May 6, 2022

Hey all...

Update

I have FINALLY had some time to look at this!

I'm actually on a new project where they need something like Axios Actions – but we're using TypeScript – so I just started playing with some raw code.

I haven't compared what I've written with AA's API, but the solution is pretty cool I think...

WIP

Take a look at this WIP (all in TypeScript):

The key is a modified token syntax; append a ! to the token to signify "this is for the URL only":

PUT posts/:id!

If an object payload is passed, that key will be removed.

Also, I'm working on a modified method signature so you can pass multiple arguments:

posts.update(1, { title: 'Hello World' })

It supports:

  • as many token variables as required for the URL
  • an optional final Object payload

I've not fully compared the existing API yet.

Examples

Here's how you would set up read and update endpoints:

const users = makeEndpoints('https://jsonplaceholder.typicode.com', {
  search: 'users?:key=:value',
  read: 'users/:id',
  update: 'PUT users/:id!',
}, options)

And here's the new flexible way of calling them:

// pass multiple values, they will be used to replace tokens in order
users.search<User[]>('username', 'Bret').then(users => {
  console.log('search:', users.map(user => user))
})

// if you pass an object as the last parameter, it will be used as the payload
users.update<User>(5, { username: 'Steve' }).then(user => {
  console.log('update:', user)
})

// or, just pass the whole payload; tokens with a `-` will remove the value from the payload
users.update<User, User>({ id: 5, username: 'Steve' }).then(user => {
  console.log('update:', user)
})

Next steps

If this token syntax seems like a good solution (and you are still using the lib!) I will look to integrate it.

If you think this won't work, let me know and we can go from there.

I will also look to port AA to TS at some point too.

Sorry again for the huuuuuge delay! 🙏

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

No branches or pull requests

6 participants