-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Initial run of network layer #4
Conversation
@jbaxleyiii should we be worried about the patent clause in Relay? Might it be worth avoiding taking parts from it? Maybe I'm overthinking it. @zol looks like the CLA bot is broken, it's reporting not signed even though it is according to the website. |
|
||
constructor(uri, opts = {}) { | ||
if (!uri) { | ||
throw new Error('A remote enpdoint is required for a newtork layer'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endpoint
@stubailo I'll take a look at the patent. I can make it more generic as well to avoid any complications |
@jbaxleyiii I dunno if the patent thing is a big deal. Maybe I should consult with someone who knows about this stuff to see how much we should care. At the end of the day everyone uses React and it has a similar patent grant. |
@stubailo that sounds good. Keeping it compatible as a relay network layer would be cool, but I don't know how much value it would add since it this layer will probably stay bare bones. If we don't worry about it, we can change the Along that note, I'd like to keep this PR open until the first run of the actual client API is started. I think the |
Yeah makes sense to me. Ensuring compatibility with the Relay client doesn't sound like a high priority right now because I haven't seen that many libraries written for it yet. |
@jbaxleyiii I think the caching wouldn't be integrated into the request/network layer right? I'm thinking:
Where the cache and network layer are swappable, and the client uses both and mediates the communication. The network layer just does queries when the client wrapper deems it necessary? |
@stubailo yep! That was what I was going to work on next. That also potentially allows us a few other options:
My thought was to do a quick first run of the client so the |
Yep, sounds good to me. One thing I'm looking into now is the ability to, given a cache and a query, figure out the set of queries needed to fetch the data that isn't in the cache yet. Sounds like that will be useful to avoid fetching data we already have.
Definitely interested in what this looks like! |
@stubailo I think the client api is a larger build out than just the network layer. I'm going to refactor this slightly tonight to make it a little cleaner / remove some the relay code. Then we should be good to merge it through and start proper on the client api |
Yep, I completely agree. I don't think this PR should include anything but a network layer implementation. Also we should agree on a design for the client before we start implementing one, since it's kind of what brings it all together. |
@stubailo this should be good to go code wise. At some point we should run a local server for testing instead of using the swapi graphql. |
Initial run of network layer
Use express-graphql GraphiQL feature
Initial run of the network layer to allow for remote requests. This interface will be used by the end client API to get the benefits of the caching system and slimmer requests.