Skip to content
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

Avoid running after/enrich without a db effect #511

Merged
merged 1 commit into from
Jun 9, 2019

Conversation

frenata
Copy link

@frenata frenata commented Oct 28, 2018

Previously both standard interceptors acted upon the db coeffect if
no effect was available. Per #453 and related discussion, there is little
reason in general to do this, since this context means that the app-db
should not change.

Both functions are changed to explicitly check for the presence of the
db effect. If it is not present in the context, the interceptor
function is not run.

The previous functionality around nil and false values for the db
effect is preserved: the interceptors are run in this case. Tests are
added where needed to verify the behavior of both functions in the case
of non-present, falsey and truthy values.

The relevant docstrings are tweaked to accommodate this change.

Closes #453.

Previously both standard interceptors acted upon the `db` *coeffect* if
no *effect* was available. Per day8#453 and related discussion, there is little
reason in general to do this, since this context means that the `app-db`
should not change.

Both functions are changed to explicitly check for the presence of the
`db` effect. If it is not present in the context, the interceptor
function is not run.

The previous functionality around `nil` and `false` values for the `db`
effect is preserved: the interceptors *are* run in this case. Tests are
added to verify the behavior of both functions in the case of
non-presence, falsey and truthy values.

The relevant docstrings are tweaked to accommodate this change.

Closes day8#453.
@superstructor superstructor self-requested a review June 9, 2019 13:57
@superstructor
Copy link
Contributor

Having read the prior discussion in #453, #447 and #450 and reviewed the code I am satisfied this change implements an agreed change in behaviour so merging. Sorry it took awhile to get to this and thanks for the PR @frenata !

@superstructor superstructor merged commit 22e8d17 into day8:master Jun 9, 2019
@mike-thompson-day8
Copy link
Contributor

mike-thompson-day8 commented Jul 10, 2019

@superstructor We need to back this out and release a new version. This is a breaking change. Some working code of ours stopped working on the upgrade from 10.6 to 10.7.

If anyone wants this feature of not running the interceptor when there's no :db effect, it should be introduced as a different interceptor with a different name (enrich2?) or something. We can't break existing code.

@mike-thompson-day8
Copy link
Contributor

mike-thompson-day8 commented Jul 15, 2019

@frenata
I'm really sorry we had to back this out after you put the effort in, and got this PR perfect.
It is my fault that this has happened. I should have recognised the problems with this change before you went to the effort. I just didn't think through the implications of the breakage correctly.

The best approach from here is to leave the existing enrich and after interceptors unchanged, and to add two new interceptors enrich2 and after2 (probably not their final names) which have the alternative behaviour. There is a valid case for both approaches.

@frenata
Copy link
Author

frenata commented Jul 15, 2019

@mike-thompson-day8 No big deal, these things happen!

I'm happy to help with further work in this area if that would be helpful, but otherwise am happy to have had a chance to dig around in the re-frame internals. It's a great project that I was happy to have used at work and I was interested in giving back in some small way.

@superstructor
Copy link
Contributor

This has been reverted in 0.10.8. Sorry for the inconvenience. I misjudged the change as low risk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only run after and enrich interceptors against an effect db, not coeffect db
3 participants