-
Notifications
You must be signed in to change notification settings - Fork 71
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
Improvements for Web Components #1444
Improvements for Web Components #1444
Conversation
Improvements that allow applications with Web Components to run with a lower overhead.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1444 +/- ##
==========================================
+ Coverage 49.42% 50.03% +0.61%
==========================================
Files 9073 8946 -127
Lines 80086 79229 -857
==========================================
+ Hits 39582 39645 +63
+ Misses 40504 39584 -920 ☔ View full report in Codecov by Sentry. |
Is there a typo or two in https://github.com/marschall/Seaside/blob/master/repository/Seaside-WebComponents-Core.package/WAHeadlessComponent.class/README.md ? Also, I wouldn't mind if the classes that are useful for SPA's are not buried, but more generally available. Perhaps in a SPA package? |
Probably a rendering issue by GitHub. Check out the raw content
What classes would you be interested in?
Maybe, but only if we have more than one class to go there. |
Got it. Thanks
If I understand things correctly, i see
as candidates. Maybe something like Seaside-SingleContinuation-Core as a name? Or maybe rename Seaside-WebComponents-Core as such? Cheers, |
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.
Looks good to me. Thanks for this!
I made a couple of code style suggestions in the detail comments.
I was first expecting a "single continuation session" to always reuse the same continuation, like what happens with ajax requests. I do a similar thing in WATurboRenderPhaseContinuation
. However, I now realise that because the webcomponent re-renders its entire fragment, it's better to throw away the old rendercontext and indeed create an entire new continuation.
...ory/Seaside-Tests-WebComponents.package/WASingleElementCacheTest.class/instance/testClear.st
Outdated
Show resolved
Hide resolved
...easide-Tests-WebComponents.package/WASingleElementCacheTest.class/instance/testKeyAtValue.st
Outdated
Show resolved
Hide resolved
...e-Tests-WebComponents.package/WASingleElementCacheTest.class/instance/testKeysAndValuesDo.st
Outdated
Show resolved
Hide resolved
...tory/Seaside-Tests-WebComponents.package/WASingleElementCacheTest.class/instance/testSize.st
Outdated
Show resolved
Hide resolved
...ory/Seaside-Tests-WebComponents.package/WASingleElementCacheTest.class/instance/testStore.st
Outdated
Show resolved
Hide resolved
...itory/Seaside-WebComponents-Core.package/WASingleElementCache.class/instance/at.ifAbsent..st
Show resolved
Hide resolved
...aside-WebComponents-Core.package/WASingleElementCache.class/instance/keyAtValue.ifAbsent..st
Show resolved
Hide resolved
@@ -0,0 +1,4 @@ | |||
processing | |||
continue | |||
"do not capture state" |
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.
Side-note: since we do not capture state, do we need the contents of the states
instvar in this class?
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.
I see what you meant. The question comes down to how much we want to tie WAFragmentRenderPhaseContinuation
to WAFragmentRenderPhaseContinuation
. Right now the states
inst vars are still filled with WASnapshot
instances due to two reasons:
WAFragmentRenderPhaseContinuation
inheritsWARenderLoopContinuation >> #createActionContinuationWithContext:
and send#snapshot:renderContext:
to the action phase continuation classWAWebComponentActionContinuation >> #handle:
still sends ends up inWASessionContinuation >> #handle:
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.
It was not a blocking comment but maybe something to look at further down the road.
9078882
to
f6be4f2
Compare
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.
Added responses to review comments
Improvements that allow applications with Web Components to run with a lower overhead.
In theory these classes could be used in general for "back button less" Seaside applications. However this is quite an advanced topic so I think it's ok to "burry" these in the WebComponents package. Let me know what you think.