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

Execute queries in Fibers to support yielding APIs. #92

Merged
merged 3 commits into from
Aug 14, 2016

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Aug 12, 2016

This change makes the global Promise constructor run all its .then callbacks using Fibers drawn from a pool of recyclable Fiber objects. In other words, any code that runs during query evaluation can now call any Fiber-based Meteor API.

I removed the "no-var-requires": true, lint rule because import Fiber = require('fibers') is the only way to import the Fiber constructor.

The meteor-promise npm package now has its own .d.ts file to support this usage.

@benjamn
Copy link
Member Author

benjamn commented Aug 12, 2016

I have to say: importing non-TypeScript npm packages into TypeScript is a huge pain. This PR took me 1.5 hours instead of ten minutes because of that nonsense. I don't know what I would have done without Visual Studio Code to ease some of the pain.

@@ -32,7 +32,19 @@ export interface QueryOptions {
formatResponse?: Function;
}

// Make the global Promise constructor Fiber-aware.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to move these four lines either to the meteor-integration package or Meteor itself?

The Promise used by Meteor will already be Fiber-enabled, so calling
makeCompatible in the tests is just simulating running in a Meteor
environment.
@helfer helfer mentioned this pull request Aug 13, 2016
@helfer
Copy link
Contributor

helfer commented Aug 14, 2016

Awesome! Really glad that Fibers didn't have to become a dependency for this to work.

@helfer helfer merged commit 4c82b8e into apollographql:master Aug 14, 2016
abernix added a commit that referenced this pull request Jul 3, 2019
…ely.

While originally introduced in
#92 to accommodate
Meteor, it seems that we're no longer making `Promise`s Fiber-aware using
the `meteor-promise` library which leverages `fibers` under-the-hood.

Since this package seems to be the sole reason that bootstrapping is failing
on Node.js 12.  To support Node.js 12, we could update to `fibers@4` which
accommodates changes to the V8 API which have been made in recent Node.js
versions, the `fibers@4` implementation is only compatible with Node.js 10
and 12 and is incompatible with Node.js 8, which we still actively support
(though not for much longer!).
abernix added a commit that referenced this pull request Jul 3, 2019
* Start running tests on Node.js v12.

While not currently a so-called "Long-Term-Support" (LTS) release, Node.js
v12 is on target to become LTS in October 2019 and Node.js 8 will end LTS
earlier than previous even-numbered Node.js releases.

See the Node.js schedule: https://github.com/nodejs/Release#release-schedule

With that only a few months away, it's time to start getting a read on
whether we're going to pass tests in that new engine.

* Remove `meteor-promise` and `fibers`, which appear to be unused entirely.

While originally introduced in
#92 to accommodate
Meteor, it seems that we're no longer making `Promise`s Fiber-aware using
the `meteor-promise` library which leverages `fibers` under-the-hood.

Since this package seems to be the sole reason that bootstrapping is failing
on Node.js 12.  To support Node.js 12, we could update to `fibers@4` which
accommodates changes to the V8 API which have been made in recent Node.js
versions, the `fibers@4` implementation is only compatible with Node.js 10
and 12 and is incompatible with Node.js 8, which we still actively support
(though not for much longer!).
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants