RFC: Primary namespace: ActiveModel::Serializer, ActiveModel::Serializers, vs. ActiveModelSerializers

[maurogeorge]

Rendered.

Following the #1298 (comment) create a RFC about namespace.

I created the RFC based on the Ember RFCs.

Thanks for the feedback ❤️

[bf4]

major 👎 this. will. cause. pain. Plus, we all agree, I think, that serializers has got to go as the main namespace.

[beauby]

@bf4 When you get a second, could you expand on the kind of pain this will cause?

[maurogeorge]

@bf4 when you say "serializers" what is the namespace? It is ActiveModel::Serializers?

[bf4]

re namespacing see #1310 (comment) @beauby re: pain... it's so obvious to me and all the work we've done together that I'm having a hard time thinking of an answer. Like using classes as namespaces is buggy, like how not everything is a Serializer, like how Serializers don't serialize and is generally a bad name, like how it's not even the core concept of the library anymore... does that mean anything? Does the naming comparison in the other comment help?

[bf4]

In any event, I'm very strongly against changing the name of the library to ActiveModel::Serializer unless someone can make a convincing argument

[bf4]

@maurogeorge

Even with the pain I think this is a good time to do that, since the AMS does not reach 1.0, if we will follow Semantic Version it is the right time to break things.

breaking things in pre-1.0 is pretty consistent with semver, unless I misunderstand you

[NullVoxPopuli]

we can always deprecate in 0.11, for example and remove the aliases to the old namespace in 0.12 (or 1.0 -- not sure what the release schedule looks like)

It's pretty easy to alias classes :-\

[maurogeorge]

@NullVoxPopuli @bf4 I agree with both of you about the semver, that if we will change the namespace we are in a good place and time to do that.

[maurogeorge]

@bf4 about the name I want change to ActiveModel::Serializers the argument is to follow the Rubygems recomendation and with that keep all the AMS code into a single namespace.

[maurogeorge]

@bf4 @NullVoxPopuli @beauby thanks a lot for your time and attention at this discussion ❤️

[beauby]

So what are the pros and cons about using ActiveModelSerializers or ActiveModel::Serializers (plural) as the base namespace? The latter kinda makes sense to me in the sense that it is inside the ActiveModel namespace.

[maurogeorge]

@beauby I think the trade-offs are only about preference about the name in this case. I vote on ActiveModel::Serializers (plural) since we are adding functionalities to the ActiveModel so this way we are following the Rubygems recommendation about naming:

Use dashes for extensions

If you’re adding functionality to another gem, use a dash. This usually corresponds to a / in the require statement (and therefore your gem’s directory structure) and a :: in the name of your main class or module.

The expected gem name, in the gemspec is active_model-serializers but I don't think we need to change this too, we can change the other things we can change without the need of a new gem on Rubygems.

Gem name	Require statement	Main class or module
active_model_Serializers	active_model/serializers	ActiveModel::Serializers

All these change are related only with the code, keeping the Rubygems the same way.

Active Model for example follow the same idea the gem name on gemspec is activemodel and to the end user is:

Gem name	Require statement	Main class or module
activemodel	active_model	ActiveModel

As you can see we do not require activemodel(the gem name in gemspec) insted we use active_model. I think we are good to go following the above table and keep the gem name on gemspec the actual name.

[NullVoxPopuli]

👍

[maurogeorge]

I think we are reaching a common sense here, as we can see at this comment.
If I am not wrong:

• @NullVoxPopuli agree with these changes.
• @beauby your agree and your concern is around ActiveModelSerializers or ActiveModel::Serializers (plural). I hope I clarified something about it on RFC: Primary namespace: ActiveModel::Serializer, ActiveModel::Serializers, vs. ActiveModelSerializers #1310 (comment)

If I was wrong please clarify here.

@bf4 after the latest discussions, what is your opinion about this?

[bf4]

@maurogeorge wrote:

I vote on ActiveModel::Serializers (plural) since we are adding functionalities to the ActiveModel so this way we are following the Rubygems recommendation about naming:

The expected gem name, in the gemspec is active_model-serializers but I don't think we need to change this too

This logic is backwards. The gem name is ActiveModelSerializers and the file in lib is active_model_serializers.rb containing the namespace ActiveModelSerializers. The RubyGem convention is for a gem and lib file of those names to put the core library functionality in lib/active_model_serializers (except for some rails engine-related conventions).

The initial name of the class in Rails was ActiveModel::Serializers -> ActiveModel::Serializer and the gem was originally named by @steveklabnik as ActiveModelSerializers, then renamed to ActiveModel::Serializer, then referred to as ActiveModel::Serializers, though the gem name didn't change, and recently we started thinking about what the name of the gem should be

Now, Rails has ActiveModel::Serializers::JSON and ActiveModel::Serialization all of which makes me think we're better off taking advantage of the gem already having a different name and prefer to use it. For example, what would you call ActiveModelSerializers::Model? ActiveModel::Serializer::Model? (Not to mention that using a class as a namespace causes load-order issues). ref my thoughts on naming and their reasons #1310 (comment)

cc @kurko @joaomdmoura @bolshakov @bacarini @mateomurphy @guilleiguaran @spastorino @josh

[bolshakov]

I like the idea of following rubygems naming convention. But I think it would be better to have namespace not related to the rails internals.

[maurogeorge]

@bf4 the logic I followed about the name ActiveModel::Serializers (plural) it was the same followed on Active Model.

About the name I wrote about it on the RFC at the motivation section

Using a class as a namespace be a bad idea, I agree, this not proposed on this RFC.

Thanks to mention and bring more people to the discussion. Let's wait for more feedback and different vision around this.

[bf4]

Right, but your vision doesn't include the whole story or renaming activemodel::serializer (which is no longer a good name for what the class does as i discussed in the notes column)

[maurogeorge]

@bf4 what you mean with whole story? I try expose my points on the RFC and with the comments, I think the more clarifying are 1 and 2.

Renaming ActiveModel::Serializer it is a real thing we need to discuss, I added this to the RFC but as a first step we can simply add this to the namespace ActiveModel::Serializers::Serializer and on a second moment rename this to a better name, but at this point all the code will be running under ActiveModel::Serializers.

[NullVoxPopuli]

Yeah, activemodel::serializer shouldn't be a class, and when it's not a class, I think it solves the problem, and there isn't anything left to discuss aside from personal preference. :-\

I'm very in favor of the ActiveModel::Serializers namespace as it means that AMS is an extension to the rails suite of tools, but also that because no part of the namespace is a class or module that AMS relies on, the namespace ActiveModel and Serializers shouldn't conflict with any non-rails projects.

[NullVoxPopuli]

I think it's also important, if the direction is to get adopted by rails officially -- which I think would be a huge benefit, esp for rails-api apps.

[bf4]

@maurogeorge

what you mean with whole story? I try expose my points on the RFC and with the comments, I think the more clarifying are 1 and 2.

You didn't discuss the 'adapters'

@NullVoxPopuli

Yeah, activemodel::serializer shouldn't be a class, and when it's not a class, I think it solves the problem, and there isn't anything left to discuss aside from personal preference.

The two are intertwined. And if I thought it were a matter of personal preference, I wouldn't bring it up. This is about 1) lessening complexity 2) shrinking the hierarchy 3) future-proofing against namespace collisions between this lib and rails, both of which would build on a second level namespace they own. It would be the only part of rails that doesn't have its own namespace 4) making things more consistent 5) admitting that ActiveModel::Serializer is no longer a good class name for what AMS does, nor is it a good primary namespace 6) deciding to what extent we want to focus on the name 'Serializer' given that the term has narrowed in scope and changed in meaning since it was first used. I included examples of this in the notes for each name #1310 (comment)

[beauby]

The only possible issue with ActiveModel::Serializers is that it already exists in rails (it contains one class, namely ActiveModel::Serializers::JSON). On the other hand, ActiveModelSerializers does not look very appealing to me.

[maurogeorge]

@bf4 thanks for clarifying the question. About adapter today they live in something like ActiveModel::Serializer::Adapter::JsonApi at the first moment to keep the things more easy to change we can keep this on ActiveModel::Serializers::Serializer::Adapter::JsonApi(as you can see keep the actual naming change only the base namespace from ActiveModel::Serializer to ActiveModel::Serializers) yeah a bad name, I know. But for less complexity on this change I think we can do this on some steps:

• Create the ActiveModel::Serializers namespace;
• Move all content under ActiveModelSerializers to be under ActiveModel::Serializers, the logger is on this step;
• Move all content under ActiveModel::Serializer to be under ActiveModel::Serializers, the adapter is on this steps;
• Move all content under ActiveModel to be under ActiveModel::Serializers, the SerializableResource is on this step ;
• Now that all the code lives under ActiveModel::Serializers we can:
  • create a better name to the ActiveModel::Serializers::Serializer keeping in mind only to keep this in the same namespace
  • create a better name to the ActiveModel::Serializers::Serializer::Adapter::JsonApi probably remove this from the ActiveModel::Serializers::Serializer and do something like ActiveModel::Serializers::Adapter::JsonApi keeping in mind only to keep this in the same namespace

As you can see the idea still the same even if this a adapter or other class/namespace.

[bf4]

@maurogeorge ok, so you still see the Adapter being inside the Serializer namespace? That doesn't fix anything. It just makes the namespace one level deeper for everything and ties the library to a namespace that this library was removed from

[maurogeorge]

@bf4 no. The adapter at the end will be in ActiveModel::Serializers::Adapter::JsonApi just to make the transition easy, we can make this on steps, on separate PRs.

IMO this fixes the 3 different namespaces we use today.

[bf4]

Ok, @maurogeorge so, do we totally agree on everything in #1310 (comment) except for whether we use ActiveModelSerializers or ActiveModel::Serializers for the primary namespace and gem name? For me, how we intend to use it is part of which one to use. (Besides whether or not we should change the gem name.)

If we change the gem name to ActiveModel::Serializers, how would you require/load it? We won't be able to require active_model/serializers since that already exists (and would also imply changing the gem name to active_model-serializers). For me, this is a big reason to avoid re-using the Rails namespace, along with no other parts of Rails being defined under namespaces for other parts.

[maurogeorge]

@bf4 I do not totally agree with #1310 (comment) since in this comment the classes are defined on 2 namespaces ActiveModelSerializers and ActiveModel (the ActiveModel::SerializableResource) and are defined new classes like ActiveModelSerializers::Current and ActiveModelSerializers::V08 and others classes too, about this new classes I don't have a strong option, since I am trying to focus here in the actual classes and to the main namespace.
I think more in this way 1 and 2.

Yes we can require active_model/serializers, I don't find a file under lib/active_model/serialzers.rb on AMS or on Active Model. I am missing something here? And we do not need to rename the gem, we can follow the same idea Active Model did.

[bf4]

Should we try to move this discussion to a Google Doc? It might help organize it better

[maurogeorge]

I don't think change tools here will solve anything.

This PR was created 2 weeks ago, one week ago was called more people to the discussion. I think we can try define what the way we will follow here.

I think actually we have the following opinions about the namespace

ActiveModel::Serializers

• Me
• @NullVoxPopuli
• @beauby

ActiveModelSerializers

• @bf4
• @bolshakov

If I was wrong please clarify here.

How can we proceed based on that?

Thanks for the feedback guys

[NullVoxPopuli]

I think it makes sense to model after rails' ActiveModel::Serializers
but, I remember a question (don't remember if this was answered): how do you require the gem, if rails includes that namespace?

I don't remember if it was answered or what the answer was, but I think as long as we keep the gem name, we'll be fine.

(I think we could keep the gem name, active_model_serializers)

[maurogeorge]

@bf4 thanks ❤️

In fact, when I wrote about that we have the ActiveModel::Serializers::JSON and ActiveModel::Serializers::Xml inside the ActiveModel::Serializers.

About the content I will update when I work on the project again, probably after the assert_ helpers

[maurogeorge]

@bf4 @beauby @NullVoxPopuli guys, sorry for the delay. I updated the RFC following the #1310 (comment)

This is ok for you?

Thanks

[bf4]

Reading on my phone, looks good. Did you see the prehistory I added to the changelog? Ams was a reverted rails pr that was supposed to incubate outside of rails, but never got pulled back in, hence tight namespace coupling

[maurogeorge]

@bf4 cool, it is something that need to be made before the merge?

Thanks

[bf4]

this should be filled int now, right?

[maurogeorge]

@bf4 Thanks for the review.
I updated the docs, following your last comments, I think we are good to merge.

[bf4]

in a future pull request. For example, we can rename this to 'Resource'

[maurogeorge]

@bf4 updated, please take a look when you have some time.

[bf4]

@rails-api/ams LGTM. ok to merge?

[bf4]

Merged. Enough time passed without objection

[maurogeorge]

Thanks for the merge ❤️