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

Missing method greaseNext:putAll:startingAt: #33

Closed
obi068 opened this issue Aug 15, 2024 · 9 comments
Closed

Missing method greaseNext:putAll:startingAt: #33

obi068 opened this issue Aug 15, 2024 · 9 comments

Comments

@obi068
Copy link

obi068 commented Aug 15, 2024

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
Copy link
Member

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
Copy link

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
Copy link
Member

@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
Copy link
Author

obi068 commented Aug 16, 2024

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
Copy link

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
Copy link

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

kurtkilpela added a commit to kurtkilpela/Seaside that referenced this issue Aug 16, 2024
FastCGI uses GRUtf8CodecStream instances. A missing selector was not detected by other test cases which use WriteStream instances.

See: GsDevKit/Grease#33
@kurtkilpela
Copy link

Added a test in SeasideSt/Seaside#1442 to test this issue w/ a GRUtf8CodecStream.

@jbrichau
Copy link
Member

jbrichau commented Aug 18, 2024

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
Copy link
Author

obi068 commented Aug 19, 2024

Issue seems to be fixed after reloading all the packages,

jbrichau pushed a commit to SeasideSt/Grease that referenced this issue Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants