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

Rack-flash incompatible with latest rack #679

Closed
nesquena opened this Issue Sep 24, 2011 · 10 comments

Comments

Projects
None yet
3 participants
@nesquena
Member

nesquena commented Sep 24, 2011

As explained here in this commit: 428f06c

RackFlash doesn't work with latest rack, and this code is based on sinatra-flash which @rkh suggested to use.

Some concerns to consider:

What about situations like this old issue: https://github.com/padrino/padrino-framework/pull/482/files where people use other stores for their flash? Just break compatibility in this point release? Is this the fix for the issue here: https://github.com/padrino/padrino-framework/pull/482/files ? I also use flash.now in my apps which this doesn't support. My vote is to either get this fixed in rack-flash or use another gem like https://github.com/SFEley/sinatra-flash that supports flash.now and hopefully doesn't break people's apps.

Also all our old apps have rack-flash installed as a dependency. This will basically mean that when people upgrade to padrino 0.10.3 (and keep an older version of rack) that they will have flash unwittingly rewritten by this with no way to disable it and use rack-flash except to disable the helpers module. @achiu what do you think?

Also, build was failing with these commits: http://travis-ci.org/#!/padrino/padrino-framework/builds/182010

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Sep 24, 2011

Member

@nakajima Any chance you can upgrade rack-flash to support the latest rack version?

Member

nesquena commented Sep 24, 2011

@nakajima Any chance you can upgrade rack-flash to support the latest rack version?

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Sep 24, 2011

Member

My problem with rack-flash is that I don't want middleware to unconditionally tamper with a) Rack::Builder and b) the app class. (by the way, a) is the thing that breaks at the moment) So even when Rack::Flash is fixed now, both changes on our side (like adding Rack::Flash in front of Padrino.application instead of each Application class) might lead to breakage again, unless the implementation of Rack::Flash is completely changed in those regards. Although it does introduce reverse-compatibility issues with respect to Gemfiles, I think it is a bad choice and should be corrected.

sinatra-flash looks much better in that regard, as it supports the API of rack-flash but only relies on stable sinatra APIs.

Member

skade commented Sep 24, 2011

My problem with rack-flash is that I don't want middleware to unconditionally tamper with a) Rack::Builder and b) the app class. (by the way, a) is the thing that breaks at the moment) So even when Rack::Flash is fixed now, both changes on our side (like adding Rack::Flash in front of Padrino.application instead of each Application class) might lead to breakage again, unless the implementation of Rack::Flash is completely changed in those regards. Although it does introduce reverse-compatibility issues with respect to Gemfiles, I think it is a bad choice and should be corrected.

sinatra-flash looks much better in that regard, as it supports the API of rack-flash but only relies on stable sinatra APIs.

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Sep 24, 2011

Member

@nesquena consider that sinatra 1.3 depends from rack ~>1.3 this mean that there is no way to get it fixed out of the box.

I preferred to add our own because is dead simple fast and doesn't require .now aka extra features.

People that have different needs can use any gem that will work without problems.

@rkh highly suggest sinatra user to don't use rack-flash

Member

DAddYE commented Sep 24, 2011

@nesquena consider that sinatra 1.3 depends from rack ~>1.3 this mean that there is no way to get it fixed out of the box.

I preferred to add our own because is dead simple fast and doesn't require .now aka extra features.

People that have different needs can use any gem that will work without problems.

@rkh highly suggest sinatra user to don't use rack-flash

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Sep 24, 2011

Member

I understand and I can see the benefits of moving away from rack-flash but I am not sure we need to merge this functionality into Padrino itself. I am of the opinion that we are better off leveraging the work of others when it makes sense. I am not sure I like the idea that flash is a helper that will always be defined whenever Helpers is a registered module and that also adds route filters and a patch to redirect for every request.

What's the downside to just switch to rack-flash for https://github.com/SFEley/sinatra-flash and not require extra code or extra tests on our end? Then in the blog post, people just need to change 'rack-flash' to 'sinatra-flash' and things will work the same with a better external flash library. I will create an alternative branch that does that just to compare and test it with padrino.

Member

nesquena commented Sep 24, 2011

I understand and I can see the benefits of moving away from rack-flash but I am not sure we need to merge this functionality into Padrino itself. I am of the opinion that we are better off leveraging the work of others when it makes sense. I am not sure I like the idea that flash is a helper that will always be defined whenever Helpers is a registered module and that also adds route filters and a patch to redirect for every request.

What's the downside to just switch to rack-flash for https://github.com/SFEley/sinatra-flash and not require extra code or extra tests on our end? Then in the blog post, people just need to change 'rack-flash' to 'sinatra-flash' and things will work the same with a better external flash library. I will create an alternative branch that does that just to compare and test it with padrino.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Sep 24, 2011

Member

Ok compare changes here for my sinatra_flash branch: master...use_sinatra_flash I added in temporary deprecation notices and support for rack-flash to be phased out in a few versions. I tried this with projects with rack-flash and then tried switching to sinatra-flash and it seemed to work fine on Sinatra edge in both cases. In another release or two we can remove rack-flash completely. Thoughts?

Only change required is change: gem 'rack-flash' to gem 'sinatra-flash' and I will mention this in the blog post as part of the upgrade notes. As skade mentioned, sinatra-flash only uses stable sinatra APIs and is very unlikely to break.

Member

nesquena commented Sep 24, 2011

Ok compare changes here for my sinatra_flash branch: master...use_sinatra_flash I added in temporary deprecation notices and support for rack-flash to be phased out in a few versions. I tried this with projects with rack-flash and then tried switching to sinatra-flash and it seemed to work fine on Sinatra edge in both cases. In another release or two we can remove rack-flash completely. Thoughts?

Only change required is change: gem 'rack-flash' to gem 'sinatra-flash' and I will mention this in the blog post as part of the upgrade notes. As skade mentioned, sinatra-flash only uses stable sinatra APIs and is very unlikely to break.

@nesquena nesquena closed this in 5091a58 Sep 24, 2011

@nesquena nesquena reopened this Sep 24, 2011

@ghost ghost assigned nesquena Sep 24, 2011

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Sep 25, 2011

Member

Thanks for that, sincerely I prefer my previous way, require less code and seems a bit faster, yep hacking redirect is not the best, but I think a website has very few redirects, generally after a post/update/delete success action.

Main big problem that I've with flash is the necessary to use always flash.now, rack-flash fortunately as a feature to workaround on this sweep => true.

In my opinion a flash plugin do few things and must be dead simple, think about express, I never used so much features.

My code (that is really not written by me) doesn't use sessions until really necessary, it use sessions only on redirect.

Simple and performant.

I think guys that need some thing more more more complex can still add an external dependency (i.e. sinatra-flash)

My aim is to use less less less external deps whenever is possible.

@skade, @achiu @rkh ?

Member

DAddYE commented Sep 25, 2011

Thanks for that, sincerely I prefer my previous way, require less code and seems a bit faster, yep hacking redirect is not the best, but I think a website has very few redirects, generally after a post/update/delete success action.

Main big problem that I've with flash is the necessary to use always flash.now, rack-flash fortunately as a feature to workaround on this sweep => true.

In my opinion a flash plugin do few things and must be dead simple, think about express, I never used so much features.

My code (that is really not written by me) doesn't use sessions until really necessary, it use sessions only on redirect.

Simple and performant.

I think guys that need some thing more more more complex can still add an external dependency (i.e. sinatra-flash)

My aim is to use less less less external deps whenever is possible.

@skade, @achiu @rkh ?

@DAddYE DAddYE closed this Sep 25, 2011

@DAddYE DAddYE reopened this Sep 25, 2011

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Sep 25, 2011

Member

sinatra-flash uses only sinatra compliant stable apis and is still pretty lightweight. I am not always less external deps. I prefer leveraging the work of people for components which are not core to our framework and keeping our framework as lean as we can. flash is a simple thing which has been implemented by sinatra-flash. Why increase the code and tests we have to directly maintain in our codebase unless its necessary?

I like that if I don't want flash in an app now that I can just remove the external dep and all the flash related code is gone in one line. With your approach, the flash helpers (and the redirect patch and associated filters) are baked directly into helpers and are not easily removed from an app. In general, I prefer modular code and components that can be easily added, removed and swapped out.

Curious to hear other opinions but I am of the opinion the less code we have to add and maintain the better for non-essential functionality and keep our code lean, relying on rack and sinatra modules whenever possible.

Member

nesquena commented Sep 25, 2011

sinatra-flash uses only sinatra compliant stable apis and is still pretty lightweight. I am not always less external deps. I prefer leveraging the work of people for components which are not core to our framework and keeping our framework as lean as we can. flash is a simple thing which has been implemented by sinatra-flash. Why increase the code and tests we have to directly maintain in our codebase unless its necessary?

I like that if I don't want flash in an app now that I can just remove the external dep and all the flash related code is gone in one line. With your approach, the flash helpers (and the redirect patch and associated filters) are baked directly into helpers and are not easily removed from an app. In general, I prefer modular code and components that can be easily added, removed and swapped out.

Curious to hear other opinions but I am of the opinion the less code we have to add and maintain the better for non-essential functionality and keep our code lean, relying on rack and sinatra modules whenever possible.

@nesquena nesquena closed this in 2f99d9f Sep 25, 2011

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Sep 25, 2011

Member

Ok spoke with @DAddYE and we decided to move the simple flash implementation to padrino-contrib and then move forward with sinatra-flash. I really do think this will be the best for us, keeping our codebase just a bit smaller. People can still choose to use either implementation (just remove sinatra flash dep and require contrib flash)

Member

nesquena commented Sep 25, 2011

Ok spoke with @DAddYE and we decided to move the simple flash implementation to padrino-contrib and then move forward with sinatra-flash. I really do think this will be the best for us, keeping our codebase just a bit smaller. People can still choose to use either implementation (just remove sinatra flash dep and require contrib flash)

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE
Member

DAddYE commented Sep 26, 2011

Added also a padrino-contrib recipe.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Sep 26, 2011

Member

Awesome, looks great, thanks for the link.

Member

nesquena commented Sep 26, 2011

Awesome, looks great, thanks for the link.

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