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

Update browser.ts #20

Merged
merged 1 commit into from
May 26, 2023
Merged

Update browser.ts #20

merged 1 commit into from
May 26, 2023

Conversation

netbrain
Copy link
Contributor

Allow for multipart/x-mixed-replace and others.


Allow for multipart/x-mixed-replace and others.
@maraisr
Copy link
Owner

maraisr commented May 24, 2023

Havent heard about this one, or others. I dont want to expose myself into inadvertently supporting other mime types I dont have any knowledge about.

Can you please help me understand the implications of this change? Love to merge this, just with a tiny more clarity!

@netbrain
Copy link
Contributor Author

netbrain commented May 25, 2023 via email

@maraisr
Copy link
Owner

maraisr commented May 26, 2023

Thanks so much for that insight @netbrain. No idea if meros would work with binary data, but doesnt seem entirely out there.

Wondering actually if I should change my api to simply ignore the response header, and assume the developer knows what theyre doing and only calling into meros when they know the response will be a stream.

Am i blocking you, or can you give me a moment to think about this?

@artola
Copy link

artola commented May 26, 2023

Thanks so much for that insight @netbrain. No idea if meros would work with binary data, but doesnt seem entirely out there.

Wondering actually if I should change my api to simply ignore the response header, and assume the developer knows what theyre doing and only calling into meros when they know the response will be a stream.

Am i blocking you, or can you give me a moment to think about this?

May be just allow to pass this as option, having the actual value as fallback. Something like options.contentType ?? 'multipart/mixed'.

@maraisr
Copy link
Owner

maraisr commented May 26, 2023

Yeah that is just moving the branch inside meros, when it could be this in user land.

fetch('..')
   .then(req => {
      if (req.headers['content-type'].includes('multipart/x-mixed-replace')) {
          return meros(req);
      }

      return req.json();
   })

all that meros cares about is a boundary defined. But yeah, if multipart/ is a safe prefix that categorically defines the pattern of "parts" however its used (replace or otherwise) — then i see no issue just relaxing the condition inside. Just need to go study this moment in more depth.

@artola does this change impact anything with HC/BCP?

@artola
Copy link

artola commented May 26, 2023

@maraisr No impact at HC/BCP. We write our network layer, and we will make it work anyway, to keep our users happy ;)

@maraisr
Copy link
Owner

maraisr commented May 26, 2023

Im going to land this. The multipart/ prefix is for all things related to the general processing of the content. How the parts are interpreted is up the consumer, and out of scope for meros.

@maraisr maraisr merged commit 4ca2b8f into maraisr:main May 26, 2023
3 checks passed
@maraisr
Copy link
Owner

maraisr commented May 26, 2023

Please check out v1.3.0 all released for you 🚀 :shipit:

@netbrain netbrain deleted the patch-1 branch May 26, 2023 12:38
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.

None yet

3 participants