Discussion about Flash API

[josevalim]

Today the Flash API exits in its own module. This discussion is to make a counter-proposal to the existing API and in order to resemble more its session counterparts.

Instead of defining many modules, Plug.Conn hosts all helpers in a single module. For example, for manipulating the session, one should call:

• fetch_session/2
• get_session/2
• put_session/3
• configure_session/2
• delete_session/2

The Flash module in Phoenix however packs everything into one exclusive module. Here is a counter proposal to organize it as the session:

• Flash.call/2 -> fetch_flash/2
• Flash.get/2 -> flash/2
• Flash.put/3 -> put_flash/2
• Flash.delete/2 -> delete_flash/2
• Flash.clear/1 -> clear_flash/1

We have two options here:

1. Move the session stuff into Plug.Conn.Session, replicating how Flash is structured today
2. Move the flash stuff into Phoenix.Controller.Conn, replicating how Session is structure today in Plug

Both approaches have their vantages and disadvantages.

In any case, I would also like to discuss the current design where flash messages are stored in a list. I would vote to keep the flash as a simple kv storage instead of storing each value in a list (flash should be general purpose as it is not used only for messages).

/cc @chrismccord @ericmj

[chrismccord]

Here is a counter proposal to organize it as the session:

Flash.call/2 -> fetch_flash/2
Flash.get/2 -> flash/2
Flash.put/3 -> put_flash/2
Flash.delete/2 -> delete_flash/2
Flash.clear/1 -> clear_flash/1

This is nice since users wouldn't need to worry about aliasing or importing it. What kind of API do you see in the view? Would a @flash key be implicitly injected in the assigns?

In any case, I would also like to discuss the current design where flash messages are stored in a list. I would vote to keep the flash as a simple kv storage instead of storing each value in a list (flash should be general purpose as it is not used only for messages).

The list approach was to allow multiple registrations. I was thinking in terms of plugs where independent plugs could register a :notice, or :error. With a regular k/v approach, you either overwrite the key, or you have to know in advance that multiple registrations are possible and write your plugs accordingly to handle things as a list. At the same time, then you also need to know in the template that your value is a list and handle accordingly. The Flash API was designed to give the same same k/v style put/get, but also support lists as needed. Is this a worthy tradeoff? We've gotten by fine in Rails with just k/v, so I'm not married to the current approach, but it does have pros.

[ericmj]

I prefer the module API for both flash and session. It scales better as you add more functionality. Why does flash and session need to have similar APIs though? I don't feel they have any overlap except that both are KV stores.

[josevalim]

@chrismccord for views we would have a flash() function in Phoenix.HTML.Flash or something of sorts.

I thought the design was for multiple entries. I have two concerns:

1. It makes it a bit less general purpose
2. I am not even sure showing multiple errors messages is the best way to go usability wise

@ericmj well, having similar APIs is easier for the user. It is more consistent, less to think about, etc.

[josevalim]

Another option is to name the functions flash, put_flash and so on but still have them in Phoenix.Controller.Flash which is imported. The upside is that they are in their own entity which is to import and/or unimport.

[chrismccord]

I am not even sure showing multiple errors messages is the best way to go usability wise

I've had this thought from time to time as well, but it would be app dependent. More often than not, it will be best as a single message.

Another option is to name the functions flash, put_flash and so on but still have them in Phoenix.Controller.Flash which is imported. The upside is that they are in their own entity which is to import and/or unimport.

This might be a happy middle ground.

conn
|> put_flash(:notice, "Great Work!")
|> render("show.html")

<%= get_flash(@conn, :notice) %>

Looks nice.

having similar APIs is easier for the user. It is more consistent, less to think about, etc.

I like the Flash module since it's the same in both the controller and view. You can just alias the module and use it in both places, but I'm now used to using it this way, so there's bias there. Similar APIs and consistency for the user is the most important thing though, and the put_flash/get_flash falls in line with our other apis, so I think I would be really happy with this change once I used it in practice.

[josevalim]

Should we do the same for session? My concern for session is that it would require someone setting up a plug and importing a module, is that ok? /cc @ericmj

[ericmj]

I still prefer namespacing the functions to modules. Flash.put_flash feels ugly but I can understand it for the practical reasons.

It's OK to go to other modules for functionality that is not "core" to the Plug.Conn API. The functions on Plug.Conn today looks perfect to me except for the session functions.

[gjaldon]

I like the idea of just renaming the Flash functions to get_flash and the like so they can be imported. Are we a go to implement that?