GsDevKit / Grease

The Grease Portability Library
MIT License
0 stars 0 forks source link

Missing method greaseNext:putAll:startingAt: #33

Closed obi068 closed 3 weeks ago

obi068 commented 3 weeks ago

After migration from GS 3.6.1 to 3.7.1 and reloading all the packages, Grease, Seaside, Zinc, .. we are getting this exception:

CodecIssues

jbrichau commented 3 weeks ago

We did some changes in this area in the latest Seaside master, which is not yet in an 'official' release 3.6.0. When you did the upgrade, did you also load a different Seaside version?

In any case, this looks like a bug in the current Seaside master for GemStone, so it means we're missing a test case for this.

kurtkilpela commented 3 weeks ago

There isn't a test as far as I can tell. I planned to add the extension into the Grease repo and add a test in Seaside repo for JSStream class >> encodeLessThanIn:at:on: to cover this particular case.

jbrichau commented 3 weeks ago

@kurtkilpela Indeed, it looks the method has been missing all along because it was never needed. We did some optimization in the JSStream class which probably exposes it.

Probably a rendering test where we generate a script tag would also trigger the issue?

obi068 commented 3 weeks ago

I load Seaside with this script: Metacello new baseline: 'Seaside3'; repository: 'github://SeasideSt/Seaside:master/repository'; onLock: [:ex | ex honor]; load: #('Development Tests' 'Examples' 'Zinc' 'JSON' 'Scriptaculous' 'Tests' 'FastCGI')

kurtkilpela commented 3 weeks ago

This is actually related to the FastCGI. It appears to me that outside of using the FastCGI, the class WriteStream is used for render streams. When using the WAFastCGIAdaptor, it will use a GRUtf8CodecStream which ultimately hits the logic causing this bug. PR #34 adds the necessary method to fix this bug.

I haven't figure out how to test this yet.

kurtkilpela commented 3 weeks ago

The change has been merged into master. @obi068 can you confirm this is no longer a problem for you?

kurtkilpela commented 3 weeks ago

Added a test in https://github.com/SeasideSt/Seaside/pull/1442 to test this issue w/ a GRUtf8CodecStream.

jbrichau commented 3 weeks ago

Thanks @kurtkilpela for providing a fix and a test!

I noticed the method actually exists but is in a Pharo-specific package (in https://github.com/SeasideSt/Grease/blob/master/repository/Grease-Pharo100-Core.package/GRDelegatingStream.extension/instance/greaseNext.putAll.startingAt..st) I think this is because it used to be part of a Pharo-specific optimization but now it is being used in the Javascript "encoding" implementation. I will fix that in Grease for the future.

In the meantime, I'm trying to find out why the test is failing though: https://github.com/SeasideSt/Seaside/actions/runs/10426395500

obi068 commented 3 weeks ago

Issue seems to be fixed after reloading all the packages,