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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some ideas for a future release #28

Closed
RomainLanz opened this issue Jan 5, 2019 · 9 comments
Closed

Some ideas for a future release #28

RomainLanz opened this issue Jan 5, 2019 · 9 comments

Comments

@RomainLanz
Copy link
Contributor

RomainLanz commented Jan 5, 2019

Hey all 馃憢

First of all, thanks for this package. It's really useful and helps to keep an API consistent and simple.

I've been working with Bumblebee for some projects and found some things that could be changed to improve the developer experience. I can work on them if they are accepted.


1. Accept namespace instead of an instance of transformers

Currently, we need to require the transformer in our controller (or another transformer) and then use this require to use it.

const UserTransformer = use('App/Transformers/UserTransformer')

...

return transform.collection(users, UserTransformer)

It could be better to directly use the namespace.

return transform.collection(users, 'App/Transformers/UserTransformer')
// or via default prefix
return transform.collection(users, 'UserTransformer')

2. Rename method toArray => toJSON
When we use the fluent interface the latest method to call is named toArray(). In fact, this method transforms your object to JSON syntax, so it would be better to name it toJSON().


3. Use JS getter
When we define our own transformer we make use of JS method. In the meanwhile, using static JS getter would be way faster (~2x).

// before
class BookTransformer extends TransformerAbstract {
  defaultInclude () {
    return ['author']
  }
}

// after
class BookTransformer extends TransformerAbstract {
  static get defaultInclude () {
    return ['author']
  }
}

4. Defines the transformer for collection and item
Currently, we have only one transform method inside our custom transformer. It could be better to have a transformCollection and transformItem when you need a different schema for a collection and an item.

For the moment, you need to create two transformers ArticleItemTransformer ArticleCollectionTransformer.


Let me know what you think about them.

@rhwilr
Copy link
Owner

rhwilr commented Jan 5, 2019

Hi @RomainLanz, glad you like it 馃槂
You make some excellent points here, I really like your proposals 馃憤

The only one I feel needs a bit of clarification is "4. Defines the transformer for collection and item".
I personally never had the need to define a different schema for a collection then for an item. The transform method describes how a single item is represented and a collection is just a selection of those items. This way you can rely on that you will get the same result when you request an item as when you get that same item as part of a collection.

I'd be interested in your use case and your view on this.

Otherwise, really like your proposals and would happily accept PRs for them. Unfortunately, I'm currently really busy with work for the university, so I won't be able to spend much time on it in the next few months. But if you are able to do some work that would be awesome!

@RomainLanz
Copy link
Contributor Author

RomainLanz commented Jan 5, 2019

Otherwise, really like your proposals and would happily accept PRs for them. Unfortunately, I'm currently really busy with work for the university, so I won't be able to spend much time on it in the next few months. But if you are able to do some work that would be awesome!

Sure I'll work on those PRs during this week.

I'd be interested in your use case and your view on this.

I can understand the point of view that you define a schema for an item and a collection is a list of this item.

My use case is the item when requested alone, give a lot more information than when it's requested via a list.

As a real example, I'm creating a CMS that can handle an article in a different language.

Calling /articles gives me the following:

[
  {
    id: 1,
    headline: '...',
    thumbnail: '...',
    featured_thumbnail: '...',
    translations: ['fr', 'en']
    published_at: '...',
    featured: false,
  },
  ...
]

In the meanwhile, calling /articles/1 gives me a lot more information:

{
  id: 1,
  headline: '...',
  description: '...',
  body: '...',
  thumbnail: '...',
  featured_thumbnail: '...',
  translations: ['fr', 'en']
  published_at: '...',
  sources: [...],
  ...
}

Therefore, having only one transformer isn't possible because the schema change between one item or a collection of this item.

@rhwilr
Copy link
Owner

rhwilr commented Jan 5, 2019

Sure I'll work on those PRs during this week.

You're awesome 馃憤 馃槂

As to your example, I did have a similar case with blog posts once where I also didn't want the body of the post in the collection view. My solution was to use an availableInclude for the body. This allowed me to keep a consistent API between the item and collection, while still making it possible to load the body over the same API if needed.

But I can see how it can be useful in some cases. I would be willing to accept a PR if it keeps the existing API where transform is used for items and collections and if needed a transformCollection and transformItem method can be defined to be more specific.

@RomainLanz
Copy link
Contributor Author

After more reflexion, I'm thinking about having a variants array that let us create many transform method.

By default transform() is called, but you could ask for a specific variant which will call transformNameOfTheVariant().

This could also help when you have two different output for casual and admin.

@rhwilr
Copy link
Owner

rhwilr commented Jan 9, 2019

I like this. It is definitely a more flexible way of implementing this.
I don't see anything preventing us from implementing this, though I haven't given it too much thought.

I'm sure you will come up with something awesome! Go ahead. 馃槂

@rhwilr
Copy link
Owner

rhwilr commented Feb 22, 2019

@RomainLanz I finally have a few hours of free time, I think I'll give feature 4 "multiple transformers" a try. 馃槂

@RomainLanz
Copy link
Contributor Author

That's awesome!

Also, it seems that the code I've added to directly use the IoC instead of passing an instance isn't working.

I'll need to take a look.

@rhwilr
Copy link
Owner

rhwilr commented Feb 23, 2019

For everyone whos interested in checking out these new features: The updated documentation can be found in the next branch and a beta version for 2.0.0 is available on npm.

Feedback and bug reports are highly appreciated.

@rhwilr
Copy link
Owner

rhwilr commented Feb 26, 2019

Since all features proposed here are now implemented, I'll close this issue. Please open a new issue to report bugs in the beta version.

@rhwilr rhwilr closed this as completed Feb 26, 2019
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

2 participants