RFC: Should the new extensions template create a Rails Engine?

dustineichler commented 5 years ago

Solidus extensions is an idea whose time has past. Replacing extensions with a new pattern was discussed at length during south east solidus this year. I'd like to explore packaging and distribution without Rails::Engine as a requirement. In terms of maintainability, I believe this would simplify things immensely.

Questions, comments and concerns below.

patleb commented 5 years ago

Please expand on the pattern you are referring to (it sounds like you are suggesting to use something other than bundler or mixing up some unrelated concepts).

dustineichler commented 5 years ago

I'm talking about how we distribute extensions through solidusio or solidusio-contrib for one and two how extensions are packaged as a Rails::Engine.

How this would ultimately work... Unclear, but I've been looking at:

patleb commented 5 years ago

How this would ultimately work... Unclear

This doesn't help much and doesn't really make your case about extensions being "an idea whose time has past" or how it would help with maintainability.

Extensions are distributed as gems which bundler pulls from whatever location you specify (local path, github, rubygems, etc.). So, if you don't go through solidusio or solidusio-contrib, then you're suggesting to replace bundler with something else?

I'm pretty sure you're mixing up some concepts here. There is nothing preventing you to not use Rails::Engine, but you will have to require manually your files and do yourself all the steps that engines do take care of: initialization hooks, generators (migrations and such), assets, locales, etc.

All in all, this doesn't make a lot of sense. Is pulling some extensions into the main project (like Rails did with ActiveStorage for example) would be something that fits with what you're trying to say/expose? If not, then please give some details about the problem(s) you're trying to solve, because you're proposing a change with an unknown solution to an unknown/non-existant problem otherwise.

jarednorman commented 5 years ago

There is nothing preventing you to not use Rails::Engine, but you will have to require manually your files and do yourself all the steps that engines do take care of: initialization hooks, generators (migrations and such), assets, locales, etc.

All true, but the tool for generating new extensions makes them engines by default, and I agree with @dustineichler that this is not a good pattern to encourage.

I've tried to follow what I think are a better set of practices for building extensions on recent stuff: normal gems (not engines), no class_eval/prepend, etc.

I personally think encouraging building them like that, and potentially moving some extensions over to something more like that would be great.

patleb commented 5 years ago

@jarednorman, thanks for your input and clarifying what was the intention of the issue (if this is really what it was about), because I thought it was way more intrusive/impactful than that. I can live with a different generator for extensions which isn't integrated with Rails and ignore all the conventions, I use a different one anyway.

Right now, the only thing I think is wrong (not ideal) with the extensions is the pattern already discussed here. Otherwise, I don't see how Rails::Engine and class_eval/prepend are problematic (you seem to be aware of something that I am not).

jarednorman commented 5 years ago

I'm very surprised that you don't see an issue with class_eval/prepend. Maintaining a system of dozens of extensions where all of them can modify the behaviour of each other has been both a burden on maintainers, as well has caused many bugs.

It essentially locks even private class APIs in place because extensions may be relying on them. In talking with our previous full time maintainer, he considered it one of the more painful pieces of maintaining the project, because every store has either class_evaled some random things, or has extensions that do it, and if anyone moves logic around in core classes they're liable to just randomly break a bunch of stuff.

I see our adoption of class_eval/prepend is a significant burden when it comes to stores keeping up with latest releases, and with maintenance in general. It's certainly been my experience that well-defined extension points offer a much better experience for both store and project maintenance, and that seemed to be the consensus during the group discussions at Southeast Solidus, but I'm curious how other people feel.

patleb commented 5 years ago

I see your point. Although, the problems you brought up aren't specific to class_eval, but to an ignored/overlooked principle of writing extensible code: the open-closed principle (should be open for extension, but closed for modification).

So, for this instance, this means that you should use class_eval to modify behavior by providing some hooks and/or a public interface to implement if modifications are to be expected/encouraged and modifying/using private parts should be discouraged by being explicit about it: meaning using the private keyword and/or prefixing with an underscore _ (which would be prefered if class_eval is expected to be used). The usage you describe is a misused of class_eval. It's very easy to blame the knife when you cut yourself, but unless you evaluate the context, you might be blaming the wrong thing.

Basically, I don't see an issue with this because the cause of headache isn't class_eval per se: you'll have the same problems if you encapsulate the behavior in another class and delegate. What I mean by this is that unless you architecture your class in an extensible way, then the same problems will arise. Not only that, but you might not have other choices, but to reopen the class or call send because the encapsulation isn't the right one. Sometimes class_eval is the right call, sometimes it's not, there are a whole bunch of ways to address the same problem. I'm guessing we see class_eval in Spree/Solidus often because more often than not we need to add a table related to other existing tables and it would imply to add relations between existing models. The easiest way to do this is by reopening the model and adding has_many, has_one, belongs_to and whatnot and it doesn't violate the open-closed principle.

Regardless of all I've just said, if you really want to make your extension maintenance bulletproof and independant of how extensions are implemented by others, not not using class_eval isn't the most robust way, because it's a losing battle to impose your way to do things. What we do with our overrides regardless of using or not class_eval, ìnclude, prepend, is we have a base test helper which compare a snapshot of the file(s) we modified when we made the modifications and run a diff everytime we update the gems. This will tell you what has changed and if the modifications should be addressed.

jarednorman commented 5 years ago

@patleb We should take a step back a here, as we've gone way off into the weeds. There's a huge disconnect happening between us in this conversation: @dustineichler was trying to continue a specific discussion from Southeast Solidus, but unfortunately didn't provide any real context for those who missed out on that conversation.

I totally appreciate that you're largely happy with how the extensions currently work and as such might be worried about unnecessary changes. Let's try to keep the discussion constructive and focus on ideas that have been explicitly suggested and avoid speculating too much. No one is suggesting replacing bundler, or banning the use of class_eval.

For a little more context: at the conference there was a discussion where some people expressed pain with how we currently handle extensions, and we discussed where we thought the trouble was coming from, and how we might alleviate it.

I think the goal here should be to continue the extension conversation from Southeast Solidus and hear from more people on where they are having issues and brainstorm on how we can address those issues.

patleb commented 5 years ago

@jarednorman, you said that you were surprised that I don't see an issue with class_eval and that you were curious how other people feel... so, naturally, I expressed my point of view and gave some actionable tips. Which, as I understand now, were unsolicited and you actually meant that I was wrong to not see an issue regardless of whatever and you wanted to hear people from Southeast Solidus specifically.

You have to understand that I have an obligation (which means I would rather not) to check from time to time for issues that would impact our clients in the long run. So, if there is an issue with the title "RFC: Replace Solidus Extensions with new library package and distribution pattern", I will most certainly associate risk and money to this kind of change and I have to inform myself about what the issue actually is. So, it's a little unfortunate that issues are now used as a forum.

Also, please, if the discussion is about how extensions are architectured/configured, then use these terms in the description of the issue instead of "library package" which would mean "gems" and "distribution pattern" which would mean "bundler".

Let's try to keep the discussion constructive

Could you please expand on the non-constructive part(s) of what I wrote? I don't get it.

and focus on ideas that have been explicitly suggested and avoid speculating too much

Sorry, but there was nothing explicit about what was written in this issue, that's why I jumped in.

No one is suggesting replacing bundler

My questions weren't answered about the distribution part... so, I guess you're answering my question about bundler with this.

or banning the use of class_eval

You specifically said "I see our adoption of class_eval/prepend is a significant burden" and I was merely suggesting that it might not be the cause of the burden with an actual explanation, a scenario and alternatives.

Alright, I'm out. Have a good one!

jarednorman commented 5 years ago

Yep, all totally understandable, and I didn't mean to imply any bad faith when I suggested we keep it constructive, only that we'd got way off track from the original/intended discussion (which is fair due to the lack of explanation/context.) Have a good rest of your weekend! 😄

dustineichler commented 5 years ago

What does a solidus extension built with Rails::Engine give us that we couldn't easily replace with a standalone or simplified Ruby gem? Namespace isolation? Can't really think of anything else off the top of my head. Engine initialization hooks can be replaced with Rails::Railtie. Also worth investigating: Classboxes.

kennyadsl commented 5 years ago

I agree we should change the title of this issue and add some more context to its description. I wasn't at the conference as well and it was quite hard to understand what we were talking about, and I still have some doubts, so I'm sorry if I'm not getting it yet.

What I'd like to point out is that the current pattern used for extensions is good if we need to inject routes or other Rails layers resources (models, views, controllers, migrations, assets) into our stores.

The extension @jarednorman built is a great example of adding some business logic to a store and it is made possible because of the presence of an extension point for that particular feature into core and because the extension does not need any assets, routes, controllers or migrations to be used.

The point, I think, is not to find an alternative when we need those things into an extension, because using what Rails gives us and maintains for us is the best and more responsible way of solving that problem (or I can't see any valid alternative yet). The point is trying to only build extensions that provide only business logic, leaving to each developer the possibility/responsibility of building the rest of the things into their store on their own.

For me, this opens some questions:

Is it ok to not providing users interfaces of features we ship into extensions? I think it could work for some features but not for all of them.
With this rationale, we should have an extension point (via a configurable class) for every feature that could be changed, but I think it's not realistic to expect that or we'll end up calling a configurable class for each line of code. We could add these configurable classes every time this is required by someone that is building a store or an extension, but I'm quite sure people will prefer to use class_eval/prepend to quick fix their issues instead of submitting PRs that could be opinionated and requires some time to be merged and released in the next Solidus version.

dustineichler commented 5 years ago

@kennyadsl good points. should update. suggestion on title change? might be worth closing and moving to solidus_cmd too.

jarednorman commented 5 years ago

Integrating my extension involved adding some views/routes/controllers/models for one of the stores that I integrated it with. I very intentionally didn't provide certain things out of the box: for example, I didn't include the state machine transitions and jobs to do transaction reporting because not all the stores I'm using it with need that functionality, and doing it properly would require assuming the use of active_job.

Instead I've provided the API required to do the reporting, and am going to document how to integrate that with your store, including making clear some of the things that may be different from store to store.

On top of this, I'm considering providing the extra admin functionality as its own extension, as well as active_job based transaction reporting as another.

This is all to say that the core functionality for some extensions might belong in its own package, and the state machine transitions, routes, controllers, views, models, etc. might belong in another package for those stores that don't need anything too custom and want a "works out of the box" experience can have that as well. Using this approach doesn't change how anything works for those currently using existing extensions, but might provide some extra flexibility for the stores that want it.