Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Lift the frame morphing logic up to FrameController.reload #1192
Lift the frame morphing logic up to FrameController.reload #1192
Changes from 5 commits
c03c893
047a479
e1fde68
5427b77
9fd1e9d
a5a87e4
87c9553
07c3577
699f66f
6abb120
38e4d2c
8344e4d
a342115
1ed1dc1
f738bdb
e956dd4
fceb2cb
3af219d
a0bf9a3
f45697c
55266c7
8f0a949
6801493
795ab37
61bd384
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these are all
static
methods, is there any benefit to defining exporting aclass
? Would a collection of exported functions serve the same purpose?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanpdoyle Unlike some of the other utility files, I did it as a class because they're tightly coupled methods so on any use you'd need to import all methods. None of them stand alone. In this way, it's similar to the Cache utility class. But there is no actual state that is being instantiated so I did them static methods.
But I'm happy to change them to independent functions which are each exported. Would you like me to make that change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading through the diff in its current state, the
MorphElements
class is only ever interacted with by importing the class and invokingMorphElements.morph
directly:Since its consistently invoked with a single method, the surface area of the module's interface could be reduced to only include
morph
:Then, the module could be simplified to be a collection of module-private functions supporting a single exported
morph
function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, you're right!
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanpdoyle As I was doing some additional cleanup, I discovered that this change broke a random test in the test suite. Specifically, moving from a class to a collection of methods.
I spent a little while trying to track down the source of the bug and the only way I could solve it was to bring the class back.
The issue has something to do with
this
context as shared between methods. Themorph()
method setsthis.isMorphingTurboFrame
which is later referenced byshouldMorphElement()
, but this is intentionally an arrow function because it's provided as a callback to Idiomorph. As I was banging my head against the wall trying to figure out another way around this or threading the context through as an argument, it occurred to me: this is the problem that classes solve! :) I just mean: sharing context between discrete method calls.For now I reverted back to the class with static methods. The full test suite passes again. If you really think it shouldn't be a class, the solution needs to involve eliminating this shared state and somehow threading it through, but I don't understand the innerworkings of Idiomorph to easily do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the morphing is stateful (uses
this
), you're correct: separate functions won't be suitable.The original feedback was around the over-use of the
static
keyword. A class that's composed of allstatic
keywords is functionally equivalent to a group of functions. Any "state" preserved through assigning to athis
will be global. That's equivalent to a module-local variable.Sharing a model-local variable across all morphing will lead to sharing that state across potentially concurrent morphs. For example, if
this.isMorphingTurboFrame
is true for a frame but false for a page-wide morph, that contention could cause unexpected behavior.As a balance, what do you think of this set of changes:
That diff removes the
static
lines in favor of a bonafide Class with instances. It also exports a singlemorphElements
function for callers to use without any knowledge that it's implemented with a class instance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanpdoyle Good call, I made this change. I confirmed that the tests all still pass.
My recommendation would be that we merge in this PR and circle back to the open "turbo:before-frame-morph" element later in a follow-up since this PR is not making any changes to events.