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

convert response callbacks into a tween #2762

Closed

Conversation

mmerickel
Copy link
Member

@mmerickel mmerickel commented Sep 6, 2016

There is a backward incompatibility here in how tweens are defined. This
is consistent with the new view deriver API but may present some
frustration for anyone currently defining a tween under the EXCVIEW
as all tweens now default to over the EXCVIEW.

This incompatibility is not strictly required to implement this feature,
but if we don't then:

  1. Tween ordering is almost completely dependent on the user's
    environment and sorting behavior. If they add another tween then the
    addon tween (if left unspecified) may be shifted to a bad place.
    This is an issue unrelated to this feature... just a lingering issue with
    tweens.

  2. Another incompatibility may surface in which a user's tween would
    show up OVER the response callbacks tween, which would lead to
    inconsistent behavior versus the previous pipeline where callbacks
    were always executed after tweens.

This patch shifts tweens to be more fully specified which should be
generally considered a good thing.

Side note, the response callbacks and excview tweens are defined
with fallbacks incase one of them is overridden or unavailable for some
reason by using a list of dependencies. We should apply this pattern to
the view derivers as well to make them easier to override.

Closes #2622.

@mmerickel mmerickel force-pushed the feature/response-callbacks-tween branch from 6a4d411 to b97e552 Compare September 6, 2016 03:03
@digitalresistor
Copy link
Member

LGTM

@mmerickel mmerickel force-pushed the feature/response-callbacks-tween branch from b97e552 to 99eb168 Compare September 6, 2016 03:19
@mmerickel
Copy link
Member Author

I'm working on docs for this now.

@mmerickel
Copy link
Member Author

mmerickel commented Sep 6, 2016

There's another issue with this feature that I just noticed.

The default for request.invoke_subrequest is use_tweens=False. Previously the response callbacks would have been executed here... this changes things so that callbacks are not executed. This incompatibility concerns me.

@mmerickel mmerickel force-pushed the feature/response-callbacks-tween branch from 99eb168 to 5a7d974 Compare September 6, 2016 03:48
@mmerickel
Copy link
Member Author

mmerickel commented Sep 6, 2016

Updated the docs but I'm worried about the subreqest situation. It feels like a fairly large incompatibility. I argued when the feature was introduced to keep use_tweens=True and honestly use_tweens as an option seems like a bad idea in general (but it's there for things like the transaction manager that cannot be nested when using the threadlocal manager... which we're starting to move away from) but it's there now so we have to deal with it.

@stevepiercy
Copy link
Member

I'll review docs when the implementation is finalized. Please ping me at that time, and I'll dive in.

@mmerickel mmerickel force-pushed the feature/response-callbacks-tween branch from bf6087b to 034a8dd Compare September 13, 2016 01:35
@mmerickel mmerickel closed this Nov 19, 2016
@mmerickel mmerickel reopened this Mar 30, 2017
@mmerickel mmerickel force-pushed the feature/response-callbacks-tween branch from 034a8dd to 1585f78 Compare April 2, 2017 18:27
There is a backward incompatibility here in how tweens are defined. This
is consistent with the new view deriver API but may present some
frustration for anyone currently defining a tween **under** the EXCVIEW
as all tweens now default to **over** the EXCVIEW.

This incompatibility is not strictly required to implement this feature,
but if we don't then:

1) Tween ordering is almost completely dependent on the user's
   environment and sorting behavior. If they add another tween then the
   addon tween (if left unspecified) may be shifted to a bad place.

2) Another incompatibility may surface in which a user's tween would
   show up OVER the response callbacks tween, which would lead to
   inconsistent behavior versus the previous pipeline where callbacks
   were always executed after tweens.

This patch shifts tweens to be more fully specified which should be
generally considered a good thing.

Side note, the response callbacks and excview tweens are defined
with fallbacks incase one of them is overridden or unavailable for some
reason by using a list of dependencies. We should apply this pattern to
the view derivers as well to make them easier to override.
@mmerickel
Copy link
Member Author

I'm going to close this and port over only the changes from config.add_tween that explicitly add tweens OVER excview unless otherwise specified.

@mmerickel mmerickel closed this Apr 10, 2017
@mmerickel mmerickel deleted the feature/response-callbacks-tween branch November 29, 2020 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants