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

Promises resolving the same value are handled incorrectly #28

Closed
linkvt opened this issue May 19, 2024 · 1 comment
Closed

Promises resolving the same value are handled incorrectly #28

linkvt opened this issue May 19, 2024 · 1 comment

Comments

@linkvt
Copy link

linkvt commented May 19, 2024

Hi,

it seems that I found a bug with turbo-stream handling promises resolving the same vlaue incorrectly.

I originally opened remix-run/remix#9466 but decided to dig a bit deeper and found the problem in this package.
In short, instead of the value of the second promise (resolving the same value) an empty array is returned like this:

[...{"31":32,"33":34,"35":36},"promiseA",["P",32],"somethingStatic",0,"promiseB",["P",36]]
P32:[1]
P36:[]

Analysis

It seems to me, that flatten is used to flatten the initial array but also responses of resolved promises:

const id = flatten.call(encoder, resolved);

The problem is, that on resolving the 2nd promise this check here does not add a new value to the response, as it was already returned with the first promise:

const existing = indices.get(input);
if (existing) return existing;

The consequence is, that an empty array is returned here:

const values = encoder.stringified
.slice(lastSentIndex + 1)
.join(",");
controller.enqueue(
textEncoder.encode(
`${TYPE_PROMISE}${deferredId}:[${values}]\n`
)
);

This causes an error in the unflatten function which checks for an empty erray:

if (!Array.isArray(parsed) || !parsed.length) throw new SyntaxError();

Possible Fix

After looking through the code it seems to me, that we cannot reference previous values when unflattening the value of a promise.
Because of this my quick fix was to send the whole promise again, which is of course not optimal.

I opened a PR with a test case and a fix, see #29

Best, Vincent

@jacob-ebey
Copy link
Owner

Closing for now. Please re-open if the issue persists. Thanks for the detailed report.

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 a pull request may close this issue.

2 participants