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

RFC: POST to Fastboot #185

Closed
wants to merge 1 commit into from
Closed

RFC: POST to Fastboot #185

wants to merge 1 commit into from

Conversation

bcardarella
Copy link
Contributor

@bcardarella bcardarella commented Dec 5, 2016

@bcardarella bcardarella changed the title POST to Fastboot RFC: POST to Fastboot Dec 5, 2016
@runspired
Copy link
Contributor

runspired commented Dec 6, 2016

@runspired
Copy link
Contributor

cc @tomdale I also think you've been working on some stuff related to this.

@kratiahuja
Copy link
Contributor

Why is this an RFC in ember.js when the changes seem to be in FastBoot? I haven't read the RFC yet.

@bcardarella
Copy link
Contributor Author

@kratiahuja
Copy link
Contributor

@bcardarella Thanks, I'll read the RFC later today. FWIW, we had discussed about this a few weeks back in fastboot meeting.

@marcoow
Copy link

marcoow commented Dec 6, 2016

I'd be interested in the use case for this. What kind of server would that proxy be? Probably not Nginx or Apache but some sort of app server (Phoenix etc.)? If that server does generate the JSON that will be used by Ember Data in the FastBoot server then, that (potentially) results in pretty strong coupling between these 2 apps - might not be a problem in some cases but certainly sth. to keep in mind.

As one of FastBoot's strengths is the fact that the app runs (more or less) the same way in the browser as well as in the FastBoot server, I'd like to carefully evaluate alternative solutions before changing that basic concept. I think the fact that FastBoot's core concept is pretty simple is hugely valuable (as opposed to other systems where the app when running in Node accesses the DB directly etc.).

@bcardarella
Copy link
Contributor Author

@marcoow it could be a Phoenix app, but yes anything that can access data directly. For many large companies this is a very likely scenario. Their infrastructure may be such that there are multiple layers the Fastboot app server needs to sit behind and cannot be directly exposed. Imagine apps where thousands of requests are happening per minute. This is the scale that is being discussed and the motivation behind reducing the additional round trip to external API. At this scale every ms counts.

@bcardarella
Copy link
Contributor Author

@kratiahuja do you have a link to those meeting notes?

@marcoow
Copy link

marcoow commented Dec 6, 2016

@bcardarella: I see. Is there any actual reason besides corporate rules to keep the FastBoot server behind the app server as opposed to in front of it? Since conceptually it's irrelevant whether the Ember app runs in the browser or the FastBoot server as they're using the same Data APIs the default setup should be to host the FastBoot server in the same zone that e.g. the Web server runs in, no?

@bcardarella
Copy link
Contributor Author

Is there any actual reason besides corporate rules

I think this is an oversimplification of the some of the infrastructure that some companies have to deal with. Single choke points where all traffic flows through is necessary for many companies. Exposing multiple app servers increases the surface area that ops teams have to be responsible for.

@marcoow
Copy link

marcoow commented Dec 6, 2016

Sure, I understand that. I'm just trying to better understand the use case and the requirements leading to it. Wouldn't the single server that all traffic flows through usually be sth. like Nginx with both the FastBoot and the app server sitting behind that as opposed to the app server sitting behind Nginx and the FastBoot server sitting behind the app server? In that case Nginx would most likely not have the data that the FastBoot server needs.

I'm not sure I understand the scenario where sth. that has already loaded the data would pass the request on to the FastBoot server that would then have to load the data again. Does dockyard see setups where the app server sits between the web and the FastBoot server?

@bcardarella
Copy link
Contributor Author

sth

I must be getting old, I don't know what this word is :p

@marcoow
Copy link

marcoow commented Dec 6, 2016

I thought it was short for something but might very well be wrong ;) What I meant was some sort of server.

@kratiahuja
Copy link
Contributor

kratiahuja commented Dec 7, 2016

@kratiahuja do you have a link to those meeting notes?

@bcardarella : https://usecanvas.com/fastboot/fastboot-meeting-notes-2016-10-21/3QXIJR79kIx8hW18iobUn9 (Read the point where I asked if FastBoot should support POST requests)

@bcardarella
Copy link
Contributor Author

@kratiahuja I'll have to check our fork but I don't believe this is the same functionality that I'm describing in this RFC

@runspired
Copy link
Contributor

@kratiahuja indeed @bcardarella's RFC is for a wholly different setup (fwiw it's also different than what I thought it was when he first described it to me, but I like the creativity of the solution and feel it's good architecture).

the TL;DR is that there is a proxy/cache server in front of the fastboot server, let's call that A.

When A needs to have fastboot render a page, it turns out that A often already knows the data that fastboot will need and could provide it to fastboot. Much like how we stream payloads to client with BPR, the POST request that Brian is proposing is a mechanism for A to request Fastboot to render a page with the store pre-populated.

@kratiahuja
Copy link
Contributor

@runspired @bcardarella My sincere apologies in misunderstanding. I still haven't done my homework of reading the RFC fully 😞 I will make time to read this RFC this week.

@marcoow
Copy link

marcoow commented Dec 8, 2016

In the example where the web/app server sitting in front of the FastBoot server has access to a cache that already has all the data needed by the FastBoot server to render the requested route, wouldn't it be easier to request that cache from the FastBoot server (e.g. in an initializer or whatever) instead of from the web/app server that then sends it in the POST request body?

Don't want to seem like a destructive element in the conversation here - just like to express some concerns and explore alternative solutions. I guess the main problems I'm seeing with the suggested change are:

  • It only really helps if all of the data needed to render the route is already present. If that's not the case then the FastBoot server would still have to load the other data which I think in many cases might take as long as loading all the data.
  • Adding this to FastBoot means that we're giving up on HTTP semantics as the FastBoot server could no longer make any distinction between GET and POST requests. I'm not sure whether this has any immediate consequences (e.g. in terms of potential security problems etc.) but I'd generally have a bad feeling doing so. Also if a FastBoot server that supported POST requests was ever exposed directly (or indirectly behind a web server) to clients (and if this is built-in to FastBoot you'd probably have to expect that to happen) you'd potentially run into problems with redirects (see e.g. http://softwareengineering.stackexchange.com/questions/99894/why-doesnt-http-have-post-redirect)
  • This effectively results in implementing a reverse shoebox mechanism along with the need of defining a format for the POST that separates different show boxes etc.

@bcardarella
Copy link
Contributor Author

@marcoow I definitely appreciate the devil's advocate here. Important part of vetting the RFC :)

Your points are well taken, but they are all architectural decisions that make sense when one is in a position to decide that architecture or when you are greenfielding. In some orgs this is not the case and the fastboot app is just one of many apps that may be managed. In this scenario the first server layer can play many roles and if one of those roles can be leveraged to improve the response time of the cycle then this is something that fastboot could support.

@bcardarella
Copy link
Contributor Author

Any updates on this?

@marcoow
Copy link

marcoow commented Feb 10, 2017

Seems like this is something that would be great to discuss in person at EmberConf?

@bcardarella
Copy link
Contributor Author

Isn't the entire purpose of RFC to discuss the issue?

@marcoow
Copy link

marcoow commented Feb 10, 2017

Good point actually ;)

My take on this is I don't think it'd be great to support POST (or any request methods other than GET for that matter) in FastBoot out of the box but not stand in the way when building custom solutions (which I think should be possible already when using https://github.com/ember-fastboot/fastboot directly in your own express middleware?).

@bcardarella
Copy link
Contributor Author

@marcoow Fastboot already accepts POST requests. This RFC simply is requesting to allow the POST request to expose the body. This is a single LOC.

@bcardarella
Copy link
Contributor Author

Sorry, correction. This is 2 LOCs.

this.method = request.method
this.body = request.body

added to the Fastboot service.

@bcardarella
Copy link
Contributor Author

it would go here: https://github.com/ember-fastboot/ember-cli-fastboot/blob/master/app/services/fastboot.js#L13

The RequestObject is private in the service and we cannot override.

@bcardarella
Copy link
Contributor Author

I've made a PR: ember-fastboot/ember-cli-fastboot#354

@marcoow
Copy link

marcoow commented Feb 10, 2017

@marcoow Fastboot already accepts POST requests.

But it will only register its middleware for GET requests by default unless I'm mistaken.

I've made a PR: ember-fastboot/ember-cli-fastboot#354

I'd actually be fine with that. I don't think it's important to prevent people from using FastBoot for POST etc. requests as well - I just think that shouldn't be enabled by default as it (potentially) makes the whole architecture much more complex.

@runspired
Copy link
Contributor

Isn't the entire purpose of RFC to discuss the issue?

👏 😹 indeed.

@mmun mmun added the T-fastboot RFCs that impact the fastboot library label Aug 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-fastboot RFCs that impact the fastboot library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants