Skip to content

Conversation

@lukasdotcom
Copy link
Member

Streaming did not work for me when using ex app routes and downloading a large file through them. Not sure if this is a correct or valid way to fix it, but it does allow downloading large files.

@bigcat88
Copy link
Member

bigcat88 commented Jul 7, 2025

At first look:

The problem that stream can break createProxyResponse with current changes.

We need at least check for $isHTML in the code(ExAppGet,...) and if it true then we will skip using stream.

To keep code clean we need to pass flag isHTML in the createProxyResponse to not duplicate code and not do double check for isHTML.

@bigcat88
Copy link
Member

bigcat88 commented Jul 7, 2025

Generally this is a nice bugfix/feature. Make sure to squash commits manually(I updated branch as you had created this PR from too old AppAPI version that was not working in my setup), as I guess, this should be backported to the NC31 and NC30.

Signed-off-by: Lukas Schaefer <lukas@lschaefer.xyz>
@lukasdotcom
Copy link
Member Author

/backport to stable31

@lukasdotcom
Copy link
Member Author

/backport to stable30

@lukasdotcom
Copy link
Member Author

Generally this is a nice bugfix/feature. Make sure to squash commits manually(I updated branch as you had created this PR from too old AppAPI version that was not working in my setup), as I guess, this should be backported to the NC31 and NC30.

I squashed it to one commit and also implemented so streaming only happens when it isn't an html file.

Copy link
Contributor

@oleksandr-nc oleksandr-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for not only finding the bug, but also making a fix.

@oleksandr-nc oleksandr-nc removed the request for review from bigcat88 July 8, 2025 15:12
@lukasdotcom lukasdotcom merged commit d89bcd8 into main Jul 8, 2025
34 checks passed
@lukasdotcom lukasdotcom deleted the fix/stream branch July 8, 2025 15:26
This was referenced Jul 8, 2025
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

Successfully merging this pull request may close these issues.

4 participants