Cycle dependency between Seaside-Core and Seaside-Canvas

[marianopeck]

If you see in BaselineOf, Seaside-Core depends on no one (https://github.com/SeasideSt/Seaside/blob/master/repository/BaselineOfSeaside3.package/BaselineOfSeaside3.class/instance/baselinecommon..st#L34) and Seaside-Canvas depends on Seaside-Core (https://github.com/SeasideSt/Seaside/blob/master/repository/BaselineOfSeaside3.package/BaselineOfSeaside3.class/instance/baselinecommon..st#L30)

However... Seaside-Core DOES NEED Seaside-Canvas... as it references the class WAHtmlCanvas which is contained in Seaside-Canvas: https://github.com/instantiations/Seaside/blob/tonel-dev/repository/Seaside-Core/WASessionCookieProtectionFilter.class.st#L87

Thoughts? @jbrichau

[jbrichau]

That's indeed a dependency that was recently (and inadvertently) introduced. The WASessionCookieProtectionFilter has some rendering built-in to display a page when no cookies are allowed. Perhaps that should be an extension to the filter in the Seaside-Canvas package, indeed.

In Pharo and GemStone, this does not cause an issue. The code is loaded and the reference to the undeclared class is automatically resolved when the Seaside-Canvas package is loaded. Does this kind of undeclared classes cause issues loading in VAST?

[marianopeck]

Hi Johan,
Yes, I imagine this wouldn't be an issue on Pharo/GemStone because otherwise the CI would be screaming hahaha. It was not a problem in VAST per-se but in our Tonel importer strategy we were using to import Seaside packages. We were trying to use a "smart" tool that would detect dependencies automatically so that we didn't have to tell dependencies explicitly (we don't have Metacello). This automatic dependencies processor was who detected this cycle.

However...when we found this problem we realized that maybe it was better to set explicit dependencies based on Seaside's BaselineOf (requires: and friends). So now we switched to this explicit approach and now the problem is solved for us.

So.... up to you Johan what you want to do. From VAST perspective we are OK, we took another approached and we moved forward.

Regardless of the result, thanks for taking a look!

[marianopeck]

Hi @jbrichau

I keep thinking about this one and while I solved my original problem, I still agree with you with the "Perhaps that should be an extension to the filter in the Seaside-Canvas package". So I will try to make that change soon.

[eMaringolo]

The only way I see to solve this properly is to introduce a third package that will depend on both.

My initial approach to this was to create a Seaside-ProtectionFilter package that will depend on Seaside-Canvas (and transitively on Seaside-Core). And then modify the BaselineOfSeaside3 to load them in the Core and common.

But as I was doing this, I noticed that having a separate package only for this two classes might be overkill or at least short-sighted, then I thought about having a separate Seaside-RequestFilter package to accomodate all WARequestFilter related things (aka "middleware"). I analyzed the change and dependencies and it could be isolated without issues, and could be a good place to integrate future filters.

So @jbrichau the next PR will include this change. If you disagree with that, please let me know and I'll modify it to have only the Seaside-ProtectionFilter package.

[jbrichau]

@eMaringolo my idea was to remove rendering in the base implementation of the filter (just return http code) and move the response that renders html to a subclass.

[eMaringolo]

Well... that's a much simpler solution.
To what package should the canvas dependent subclass be moved?

[marianopeck]

I guess to Seaside-Canvas, right?

[jbrichau]

@eMaringolo I was going to take care of it but you can beat me to it as I will only be able to get back to Seaside by the end of the week. 😉
Maybe Seaside-Components ?

[eMaringolo]

BTW, I didn't move the canvas-dependent filter to Seaside-Canvas nor to Seaside-Component, both packages are pretty well defined in the scope, and I didn't want to pollute them. So I moved it to Seaside-Environment that seemed more like a mix.