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

[BREAKING] Remove pluralisation & stop magically extracting Type and ID from the first parameter #415

Open
wopian opened this issue May 19, 2020 · 5 comments

Comments

@wopian
Copy link
Owner

wopian commented May 19, 2020

Changes are broken down by package (kitsu and kitsu-core)

kitsu

What

  1. Change the first parameter behaviour of api.get, api.patch, api.post and api.delete to be a plain URL path. E.g.

    • api.get(anime/1/relationships/episodes)
    • api.patch(users/1/password, body) where the type may not be passwords
  2. Remove the second parameter of api.delete

    • api.delete('posts', 1) is redundant when you can do api.delete('posts/1')
  3. Second parameter (deserialised JSON:API) of api.patch and api.post would now require the type to be defined

    • Current:
      • api.post('post', { content: 'some content' }
      • api.patch('post', { id: '1', content: 'some content' }
    • Proposed:
      • api.post('posts', { type: 'posts', content: 'some content' }
      • api.patch('posts', { id: '1', type: 'posts', content: 'some content' }
  4. Drop dependency on pluralize

Why

  • (1/2) Allows custom URLs for JSON:API resources without creating custom requests (how to send raw request? #414)

  • (4) Significantly reduces kitsu bundle size by 30% - pluralize is 2.3 kB of kitsu's 8.2 kB.

  • (1/3/4) Pluralisation leads to ambiguity of what the real value of type in the API actually is (especially when it is not simple rules like post -> posts)

    • Proposed behaviour would be WYSIWYG (What You See Is What You Get)
  • (1/3/4) Pluralisation can get in the way of the correct value of type in some API endpoints, but not all endpoints.

    • Which results in consumers either disabling pluralisation entirely or setting pluralisation rules to mitigate the one wrong type

Pain points

  • This would break SOME GET, PATCH, POST and DELETE requests that relied on pluralisation to request the API resource

  • This would break ALL PATCH, POST requests as the data sent to the API will now be lacking the type without changes to consumer code

  • This would break ALL DELETE requests as the 2nd argument is removed, making the 3rd argument for additional headers the 2nd argument

kitsu-core

What

  1. Remove the first parameter of serialise (model / the root resources' type)

    • The second parameter (obj / the deserialised JSON:API input) now requires the type to be defined. E.g:
      • Current: serialise('posts', { id: '1', content: 'some content' }
      • Proposed: serialise({ id: '1', type: 'posts', content: 'some content' }
  2. Remove pluralTypes and camelCaseTypes options added in 9.x as with (1.) the type will already be in the resource object and should be correctly formatted already

    • For upgrades from 8.x and older it would be serialise.apply({ camel, resCase: kebab, plural }, [ model, obj ]) to serialise(obj) (with model now obj.type)
    • Instead of being extracted from the model string in the Kitsu class and passed down, where pluralisation mismatch and kebab/snakecase could be present, which necessitated these options`

Why

  • (1/2) It is only used once to set the type of a new object (along with pluralisation and camelCase conversion)

  • (1) With the logic removed from the get, patch and post methods from the Kitsu class this doesn't really make much sense to keep type separate here still

  • (1) With the proposed behaviour it would match the existing behaviour of adding the id to the new data object - extracting it from the second parameter (obj)

    • serialise will need to throw an error if type is missing
      • Only when obj is NOT null or [] (syntax to delete relationships)
      • In current behaviour it is guaranteed to have a type set

Pain points

  • This would break ALL uses of serialise as the first argument is dropped, making the input object (deserialised JSON:API) the first argument.
@Juanmcuello
Copy link

@wopian , this proposal looks good. Is it something you already started to work on?

@wopian
Copy link
Owner Author

wopian commented Jun 28, 2021

@wopian , this proposal looks good. Is it something you already started to work on?

I have not yet started on this unfortunately

@wopian wopian modified the milestones: 10, 11 Aug 11, 2022
@8area8
Copy link

8area8 commented Aug 19, 2022

important feature, kitsu is broken on my environment because of its url/type coupling during the pluralize

@wopian
Copy link
Owner Author

wopian commented Aug 19, 2022

important feature, kitsu is broken on my environment because of its url/type coupling during the pluralize

Is it broken even when you disable pluralize in the kitsu config options?

https://github.com/wopian/kitsu/tree/master/packages/kitsu#parameters

@8area8
Copy link

8area8 commented Aug 19, 2022

My environment uses plural for the urls only. So, if I disable the plural option, it reconize the resource but not the url.

I can still use api.request but it is less interesting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants