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

Inline scripts can get broken #1

Closed
dario-piotrowicz opened this issue Sep 14, 2022 · 5 comments · Fixed by #2
Closed

Inline scripts can get broken #1

dario-piotrowicz opened this issue Sep 14, 2022 · 5 comments · Fixed by #2
Labels

Comments

@dario-piotrowicz
Copy link
Contributor

Version: 1.0.1

Details

Hi @DylanPiercey, awesome small library you've created 😄

Anyways one thing I noticed as I was playing with it is that inline scripts don't seem to be fully supported, what I mean is that if an inline script gets truncated in the stream, then it won't be correctly handled by the library, as the library will either execute only part of the script and/or will present a syntax error depending on where the script got truncated.

For example I have created the following Cloudflare worker which exposes a stream which truncates a very simple inline script:

addEventListener("fetch", event => {
  event.respondWith(handleRequest(event.request))
})

async function handleRequest(request) {
  return new Response(getStream()});
}

function getStream() {
  const { writable, readable } = new TransformStream();
  writeInStream(writable);
  return readable;
}

const chunks = [
`<script>
  console.log('`,`this is streaming!');
</script>`,
];

async function writeInStream(writable) {
  const encoder = new TextEncoder();
  const writer = writable.getWriter();
  const write = str => writer.write(encoder.encode(str));

  for (const chunk of chunks) {
    write(chunk);
    await new Promise(resolve => setTimeout(resolve, 1000));
  }

  writer.close();
}

(well, the implementation there is not important, the relevant thing is that the stream is comprised of two separate chunks:
<script>console.log(' and this is streaming!');</script>)

If I run the code you present in the Repo's readme (in which of course I replace "https://ebay.com" with the worker's URL)
I get a syntax error in the console:
Screenshot 2022-09-14 at 22 44 51

The issue being that initially the script gets only partially added to the document:
Kapture 2022-09-14 at 22 48 38

I think that the script's content should be buffered and that the script should be added fully to the document in a single operation, so that the whole script will be evaluated correctly by the browser.

Naturally my example here is a silly one, but I came across this issue as I was trying to use the library and the stream I was trying to add to the dom happens to get truncated causing a syntax error and making the whole thing not work.

Have you considered this issue? and/or do you have an opinion/potential solution for it?

As I'm interested in what this library is doing, I wouldn't mind trying to add a fix and submit a PR 🙂

@DylanPiercey
Copy link
Contributor

@dario-piotrowicz ah this is a good catch. I don't have time to dig into this right away but would happily review a PR.
Probably the logic needs to be updated to avoid setting text content for <script> and probably <style> until they are complete as you say.

@dario-piotrowicz
Copy link
Contributor Author

Ah ok thanks, I'll see what I can do then! 😃👍

@dario-piotrowicz
Copy link
Contributor Author

By the way, this is what happens with styles:

Kapture 2022-09-15 at 11 31 22

They are streamed correctly and everything works ok (and there are no errors in the console)

The only issue being that only the partially streamed styles get applied, like in my gif for example you can see the black background being applied first and the green color being applied only on the next chunk.

What do you think? is this behavior ok? or should the styles be blocking and appear all in one go?

dario-piotrowicz added a commit to dario-piotrowicz/writable-dom that referenced this issue Sep 15, 2022
fix the issue which breaks scripts split in different chunks in the input stream
by applying them only after the scripts has been fully visited

resolves marko-js#1
dario-piotrowicz added a commit to dario-piotrowicz/writable-dom that referenced this issue Sep 15, 2022
fix the issue which breaks scripts split in different chunks in the input stream
by applying them only after the scripts has been fully visited

resolves marko-js#1
@DylanPiercey
Copy link
Contributor

DylanPiercey commented Sep 15, 2022

@dario-piotrowicz I think to be consistent with browser behavior the styles should be displayed all at once with the related tag. Otherwise there could be flashes of unexpected style combinations.

dario-piotrowicz added a commit to dario-piotrowicz/writable-dom that referenced this issue Sep 15, 2022
fix the issue which breaks scripts split in different chunks in the input stream
by applying them only after the scripts has been fully visited

resolves marko-js#1
dario-piotrowicz added a commit to dario-piotrowicz/writable-dom that referenced this issue Sep 15, 2022
fix the issue which breaks scripts split in different chunks in the input stream
by applying them only after the scripts has been fully visited

resolves marko-js#1
dario-piotrowicz added a commit to dario-piotrowicz/writable-dom that referenced this issue Sep 15, 2022
fix the issue which breaks scripts split in different chunks in the input stream
by applying them only after the scripts has been fully visited

also fix the same issue for embeded styles, even though there everything works
fine it is more safe and clean to apply all the styles together

resolves marko-js#1
dario-piotrowicz added a commit to dario-piotrowicz/writable-dom that referenced this issue Sep 15, 2022
fix the issue which breaks scripts split in different chunks in the input stream
by applying them only after the scripts has been fully visited

also fix the same issue for embeded styles, even though there everything works
fine it is better to apply all the styles together and avoid potential flashes
of unexpected style combinations

resolves marko-js#1
dario-piotrowicz added a commit to dario-piotrowicz/writable-dom that referenced this issue Sep 15, 2022
fix the issue which breaks scripts split in different chunks in the input stream
by applying them only after the scripts has been fully visited

also fix the same issue for embeded styles, even though there everything works
fine it is better to apply all the styles together and avoid potential flashes
of unexpected style combinations

resolves marko-js#1
dario-piotrowicz added a commit to dario-piotrowicz/writable-dom that referenced this issue Sep 15, 2022
fix the issue which breaks scripts split in different chunks in the input stream
by applying them only after the scripts has been fully visited

also fix the same issue for embeded styles, even though there everything works
fine it is better to apply all the styles together and avoid potential flashes
of unexpected style combinations

resolves marko-js#1
dario-piotrowicz added a commit to dario-piotrowicz/writable-dom that referenced this issue Sep 15, 2022
fix the issue which breaks scripts split in different chunks in the input stream
by applying them only after the scripts has been fully visited

also fix the same issue for embeded styles, even though there everything works
fine it is better to apply all the styles together and avoid potential flashes
of unexpected style combinations

resolves marko-js#1
dario-piotrowicz added a commit to dario-piotrowicz/writable-dom that referenced this issue Sep 15, 2022
fix the issue which breaks scripts split in different chunks in the input stream
by applying them only after the scripts has been fully visited

also fix the same issue for embeded styles, even though there everything works
fine it is better to apply all the styles together and avoid potential flashes
of unexpected style combinations

resolves marko-js#1
@github-actions
Copy link

🎉 This issue has been resolved in version 1.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants