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

Provide isomorphic implementation of WHATWG fetch() API. #10029

Merged
merged 7 commits into from Jun 28, 2018

Conversation

Projects
None yet
5 participants
@benjamn
Member

benjamn commented Jun 25, 2018

As described in my talk about Meteor 1.7's modern/legacy bundling system (relevant slides), the WHATWG fetch() API is a great example of a feature that requires a totally different implementation in modern browsers, legacy browsers, and Node.

The fetch() API is already widely used in the JavaScript community, and I suspect it will eventually replace the core Meteor http package for most HTTP request needs, especially since it is essentially free in modern browsers.

@benjamn benjamn added this to the Release 1.7.1 milestone Jun 25, 2018

@benjamn benjamn self-assigned this Jun 25, 2018

@benjamn benjamn requested review from hwillson and abernix Jun 25, 2018

// Write your tests here!
// Here is an example.
Tinytest.add('fetch - example', function (test) {
test.equal(typeof fetch, "function");

This comment has been minimized.

@benjamn

benjamn Jun 25, 2018

Member

Need more tests, obviously!

This comment has been minimized.

@hwillson

hwillson Jun 26, 2018

Member

No way - I like how manageable these tests are ... 🙂.

@benjamn

This comment has been minimized.

Member

benjamn commented Jun 25, 2018

If anyone else wants to help replace HTTP with fetch in Meteor core packages, just comment here first, and then open a PR targeting the isomorphic-fetch branch.

@hwillson

Woohoo! This is great @benjamn - so far so great!

// Write your tests here!
// Here is an example.
Tinytest.add('fetch - example', function (test) {
test.equal(typeof fetch, "function");

This comment has been minimized.

@hwillson

hwillson Jun 26, 2018

Member

No way - I like how manageable these tests are ... 🙂.

@sebakerckhof

This comment has been minimized.

Contributor

sebakerckhof commented Jun 26, 2018

@benjamn what would the strategy be for replacing server-side calls that are now synchronous because of fibers. Just Promise.await the fetch?

And what about error handling? HTTP package will throw if the status code is >= 400 (see https://github.com/meteor/meteor/blob/devel/packages/http/httpcall_server.js#L119 and https://github.com/meteor/meteor/blob/devel/packages/http/httpcall_client.js#L180), fetch will not do this.
So I don't think the places where you already replaced it have the same behavior as before.

@benjamn

This comment has been minimized.

Member

benjamn commented Jun 26, 2018

what would the strategy be for replacing server-side calls that are now synchronous because of fibers. Just Promise.await the fetch?

This PR is part of a longer-term plan to encourage developers to move away from fibers and towards existing web standards. So I would honestly recommend actually awaiting the fetch in an async function, or working with the Promise API explicitly, though fibers will of course still work.

So I don't think the places where you already replaced it have the same behavior as before.

Thanks for pointing that out! I don't consider fetch() a drop-in replacement for HTTP by any means, though perhaps we should be clearer about the differences.

@sebakerckhof

This comment has been minimized.

Contributor

sebakerckhof commented Jun 26, 2018

My questions were a reply to #10029 (comment). So in the context of replacing HTTP in the current core packages, are those also the recommendations? Because awaiting in an async instead of relying on fibers would be a backwards incompatible change for end-users, which Meteor usually tries to prevent.

Same thing for the errors. I agree that fetch shouldn't be a drop-in replacement for HTTP. Rather the opposite and it should just follow the fetch-spec. But handling errors differently might brake existing core packages. E.g. when they rely on a 404 throwing an error or something.

@jamesmillerburgess

This comment has been minimized.

Contributor

jamesmillerburgess commented Jun 26, 2018

I will replace HTTP in bundle-visualizer since I'm already touching it in another PR.

@benjamn

This comment has been minimized.

Member

benjamn commented Jun 26, 2018

In light of @sebakerckhof's astute feedback, and in order to make this package available for use with the bundle-visualizer (and other packages where it makes sense), I think we should try to publish the initial version of fetch soon (maybe after adding more tests), without fully migrating away from (or deprecating) http just yet.

Let's make fetch() happen!

@abernix

LGTM. Additional fetch tests seem like a good idea, and I agree with getting this out sooner rather than later, but I think we can also probably relish in certain (test) benefits we're gaining thanks to the existing tests that whatwg-fetch and node-fetch have to offer!

benjamn and others added some commits May 11, 2018

@benjamn benjamn force-pushed the isomorphic-fetch branch from ba95309 to a20f4e7 Jun 28, 2018

@benjamn benjamn merged commit 4502ad6 into devel Jun 28, 2018

3 of 18 checks passed

ci/circleci: Isolated Tests CircleCI is running your tests
Details
ci/circleci: Test Group 0 CircleCI is running your tests
Details
ci/circleci: Test Group 1 CircleCI is running your tests
Details
ci/circleci: Test Group 10 CircleCI is running your tests
Details
ci/circleci: Test Group 2 CircleCI is running your tests
Details
ci/circleci: Test Group 3 CircleCI is running your tests
Details
ci/circleci: Test Group 4 CircleCI is running your tests
Details
ci/circleci: Test Group 5 CircleCI is running your tests
Details
ci/circleci: Test Group 6 CircleCI is running your tests
Details
ci/circleci: Test Group 7 CircleCI is running your tests
Details
ci/circleci: Test Group 8 CircleCI is running your tests
Details
ci/circleci: Test Group 9 CircleCI is running your tests
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
CLA Author has signed the Meteor CLA.
Details
ci/circleci: Docs Your tests passed on CircleCI!
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment