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

fix: encoding of previously-used values #38

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

lsthornt
Copy link
Contributor

@lsthornt lsthornt commented Jul 24, 2024

I had more occurrences of #36 in my project, and I was able to isolate the issue much better. I believe this test is the absolute minimal case.

Reused values certainly work in many other scenarios, but this specific setup results in a circular reference for the third object key: data.

  • if the value for data is not an object, no failure occurs
  • if 'baz' is not used as object value in 'foo', or if it is an object value in bar, no failure occurs.

In my codebase, foo, bar, and data are all independent promises resolved from a Remix loader. When data resolves prior to bar, no failure occurs. The PREVIOUSLY_RESOLVED value must resolve prior to the data object. In doing so, it seems it taints either the values or hydrated cache in unflatten

@lsthornt
Copy link
Contributor Author

I've added what I believe to be a fix.

It seemed like the issue was with encoding rather than decoding. The object encoded after the reused value would end up decoding starting at a preceding index, leading to the circular reference, as I saw in my original issue.

The fix doesn't feel elegant, but I've given it some testing in my real project and it seems sound. @jacob-ebey hoping can take a closer and wiser look.

@nick-potts
Copy link

I'm really glad you managed to track this one down - I spent far too long trying to create a minimal reproduction.

@lsthornt
Copy link
Contributor Author

Me too, I really just got lucky with a reproducible example in my project.

@nick-potts it would be great to know if this fix resolves your issue too – any extra test time is helpful. Here is the patch I'm using:

diff --git a/node_modules/turbo-stream/dist/turbo-stream.js b/node_modules/turbo-stream/dist/turbo-stream.js
index 18027f9..2222cb7 100644
--- a/node_modules/turbo-stream/dist/turbo-stream.js
+++ b/node_modules/turbo-stream/dist/turbo-stream.js
@@ -141,6 +141,8 @@ function encode(input, options) {
                         const id = flatten_js_1.flatten.call(encoder, resolved);
                         if (Array.isArray(id)) {
                             controller.enqueue(textEncoder.encode(`${utils_js_1.TYPE_PROMISE}${deferredId}:[["${utils_js_1.TYPE_PREVIOUS_RESOLVED}",${id[0]}]]\n`));
+                            encoder.index++;
+                            lastSentIndex++;
                         }
                         else if (id < 0) {
                             controller.enqueue(textEncoder.encode(`${utils_js_1.TYPE_PROMISE}${deferredId}:${id}\n`));
@@ -161,6 +163,8 @@ function encode(input, options) {
                         const id = flatten_js_1.flatten.call(encoder, reason);
                         if (Array.isArray(id)) {
                             controller.enqueue(textEncoder.encode(`${utils_js_1.TYPE_ERROR}${deferredId}:[["${utils_js_1.TYPE_PREVIOUS_RESOLVED}",${id[0]}]]\n`));
+                            encoder.index++;
+                            lastSentIndex++;
                         }
                         else if (id < 0) {
                             controller.enqueue(textEncoder.encode(`${utils_js_1.TYPE_ERROR}${deferredId}:${id}\n`));
diff --git a/node_modules/turbo-stream/dist/turbo-stream.mjs b/node_modules/turbo-stream/dist/turbo-stream.mjs
index f95b077..7e0d25c 100644
--- a/node_modules/turbo-stream/dist/turbo-stream.mjs
+++ b/node_modules/turbo-stream/dist/turbo-stream.mjs
@@ -452,6 +452,8 @@ function encode(input, options) {
 `
                     )
                   );
+                  encoder.index++;
+                  lastSentIndex++;
                 } else if (id2 < 0) {
                   controller.enqueue(
                     textEncoder.encode(`${TYPE_PROMISE}${deferredId}:${id2}
@@ -480,6 +482,8 @@ function encode(input, options) {
 `
                     )
                   );
+                  encoder.index++;
+                  lastSentIndex++;
                 } else if (id2 < 0) {
                   controller.enqueue(
                     textEncoder.encode(`${TYPE_ERROR}${deferredId}:${id2}

@nick-potts
Copy link

Yep, gave it a shot and it works an absolute treat. Mine's probably a great stress test on it, lots of very similar data with lots of nested keys with the same name. Also passing about 50-200 promises per page load.

@lsthornt lsthornt changed the title Adds failing test for previously resolved values Fixes encoding of previously-used values Jul 26, 2024
@jacob-ebey jacob-ebey changed the title Fixes encoding of previously-used values fix: encoding of previously-used values Aug 9, 2024
@jacob-ebey jacob-ebey merged commit 84520be into jacob-ebey:main Aug 9, 2024
1 check passed
@nick-potts
Copy link

woot woot

@nick-potts
Copy link

I tried 2.2.2, but its reporting an internal error. Will get back to you when I understand it.

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.

3 participants