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 Chrome/Firefox support for ReadableStream in Request Body #1333

Merged
merged 4 commits into from Apr 18, 2018

Conversation

Projects
None yet
4 participants
@AnthumChris
Contributor

AnthumChris commented Mar 10, 2018

To the best of my knowledge, Chrome and Firefox do not yet support sending ReadableStream content in Request body during HTTP requests. I think we should remove the positive indicators until this is fully supported. Tested with Chrome 67 and FF 59.

// String test - passes
post('hello');

// ReadableStream test - fails
post(new ReadableStream({
  start(controller) {
    controller.enqueue(new TextEncoder().encode('hello'));
    controller.close();
  }
}));

// Server should respond with postBody text value, indicating it was sent and received.
function post(postBody) {
  fetch('https://dev.anthum.com/post-test/', {
    body: postBody,
    headers: {'content-type': 'text/plain'},
    method: 'POST'
  })
  .then(response => response.text())
  .then(data => console.log(data))
  .catch(error => console.error(error));
}
@evilpie

This comment has been minimized.

Contributor

evilpie commented Mar 12, 2018

Did you test with preferences enabled in Firefox?

@AnthumChris

This comment has been minimized.

Contributor

AnthumChris commented Mar 12, 2018

@evilpie Yes, the preferences/flags were enabled. Firefox sends string "[object ReadableStream]".

@Elchi3 Elchi3 requested a review from chrisdavidmills Mar 14, 2018

@chrisdavidmills

I think the main problem here is that the description is misleading — in this data I am really talking about consuming response bodies as streams, not sending requests with streams as bodies.

I think it is worth talking about both — could you update the existing description to say something like "Consuming response body as ReadableStream", and then add an extra support block to cover the other case?

@Elchi3

This comment has been minimized.

Member

Elchi3 commented Apr 17, 2018

It's not clear to me how the two blocks should look like. @chrisdavidmills can you post the JSON you'd like to see here, so that I can amend this PR? (or open a new PR to get this right and I'll close this one.) Thanks!

@chrisdavidmills

This comment has been minimized.

Contributor

chrisdavidmills commented Apr 17, 2018

Something like this?

"reponse_body_readablestream": {
  "__compat": {
    "description": "Consume response body as a <code>ReadableStream</code>",
    "support": {
      "chrome": {
        "version_added": "43"
      },
      "chrome_android": {
        "version_added": "43"
      },
      "edge": {
        "version_added": null
      },
      "edge_mobile": {
        "version_added": null
      },
      "firefox": {
        "version_added": true,
        "flags": [
          {
            "type": "preference",
            "name": "dom.streams.enabled"
          },
          {
            "type": "preference",
            "name": "javascript.options.streams"
          }
        ]
      },
      "firefox_android": {
        "version_added": true,
        "flags": [
          {
            "type": "preference",
            "name": "dom.streams.enabled"
          },
          {
            "type": "preference",
            "name": "javascript.options.streams"
          }
        ]
      },
      "ie": {
        "version_added": null
      },
      "opera": {
        "version_added": null
      },
      "opera_android": {
        "version_added": null
      },
      "safari": {
        "version_added": false
      },
      "safari_ios": {
        "version_added": "10.3"
      },
      "webview_android": {
        "version_added": "43"
      }
    }
  }
},
"readablestream_request_body": {
  "__compat": {
    "description": "Send <code>ReadableStream</code> in request body",
    "support": {
      "chrome": {
        "version_added": false
      },
      "chrome_android": {
        "version_added": false
      },
      "edge": {
        "version_added": null
      },
      "edge_mobile": {
        "version_added": null
      },
      "firefox": {
        "version_added": false
      },
      "firefox_android": {
        "version_added": false
      },
      "ie": {
        "version_added": null
      },
      "opera": {
        "version_added": null
      },
      "opera_android": {
        "version_added": null
      },
      "safari": {
        "version_added": false
      },
      "safari_ios": {
        "version_added": false
      },
      "webview_android": {
        "version_added": false
      }
    }
  }
}

Elchi3 added some commits Apr 18, 2018

@Elchi3 Elchi3 changed the title from remove Chrome/Firefox support for ReadableStream in Request Body to Update Chrome/Firefox support for ReadableStream in Request Body Apr 18, 2018

@Elchi3 Elchi3 merged commit 1b4c054 into mdn:master Apr 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@AnthumChris AnthumChris deleted the AnthumChris:stream-request-body branch Apr 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment