Skip to content
This repository has been archived by the owner on Apr 20, 2024. It is now read-only.

Fix PR #29 #34

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

code28
Copy link

@code28 code28 commented Oct 23, 2018

I liked the idea of not having a private container (Issue #26 and Pull request #29). Since there was no development since june, I took a look and fixed two little things (one of them fixes #30), so now I think it works.

So the main work for this was done by @siemensikkema, I just made two tiny fixes to make it work. :)

@codecov
Copy link

codecov bot commented Oct 23, 2018

Codecov Report

Merging #34 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@         Coverage Diff          @@
##           master   #34   +/-   ##
====================================
  Coverage       0%    0%           
====================================
  Files           8     6    -2     
  Lines         107    84   -23     
====================================
+ Misses        107    84   -23
Impacted Files Coverage Δ
Sources/Flash/Providers/FlashProvider.swift 0% <ø> (ø) ⬆️
Sources/Flash/Models/Flash.swift 0% <ø> (ø) ⬆️
Sources/Flash/Extensions/Container+Flash.swift 0% <0%> (ø)
Sources/Flash/Tags/FlashTag.swift 0% <0%> (ø) ⬆️
Sources/Flash/Middlewares/FlashMiddleware.swift 0% <0%> (ø) ⬆️
Sources/Flash/Extensions/Future+Flash.swift 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8763c73...1124b4a. Read the comment docs.

@steffendsommer
Copy link
Contributor

@code28 thanks a lot for following up on this - and we're sorry for the delay. Let's have @siemensikkema go through your PR.

@siemensikkema
Copy link
Contributor

@code28 Thanks for the PR! It seems the way the flashes are en/decoded to and from the session still uses the private container and the README still mentions that the private container should be used.
A different approach to solving this is to pass the request into the render function using the functionality from https://github.com/nodes-vapor/sugar/blob/master/Sources/Sugar/Helpers/Render%2BuserInfo.swift (see: https://github.com/nodes-vapor/sugar/releases/tag/3.0.0-beta.18). Only issue is now that the we need to remember to pass in the request but at least the view can be rendered the normal (cached) way.

@code28
Copy link
Author

code28 commented Oct 24, 2018

Well, it seems to me, that the privateContainer is not necessary anymore. I tried removing privateContainer part when en-/decoding to and from the session, and it still works. In general, I don't use private containers for flashes at all in my project and it works.

I must admit, that I don't fully understand the principle of private containers here, but I have the impression, that we can remove it here.

I can fix the readme, as soon as we found the way to go :)

@siemensikkema
Copy link
Contributor

@code28 I believe that when using the normal container you use the one that is for the event loop and therefore shared by all the requests. I'd expect you'd run into problems where flashes from different requests (different users) would appear.

@code28
Copy link
Author

code28 commented Oct 24, 2018

Well, I just tested with two different users at the same time and not a private container, but the flashes were seperately for each user...

@tonyarnold tonyarnold mentioned this pull request Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show flash messages only once
3 participants