Expose a public API to allow assets to be served in a custom way using ember serve

[kratiahuja]

This RFC attempts to resolve this issue. For more background, please read that issue.

rendered

cc: @stefanpenner @ryanone @masonwan @kristoferbaxter

[stefanpenner]

Awesome, thank you for getting this into words, now that we have something to concretely discuss, it seems like we can actually rely more on express middleware than I initially thought.

Rather than creating something similar to express middleware here, it could very likely be just express middleware. This allows us to lean on existing expectations of express, and avoid introducing unnecessary concepts.

If we rethink the existing single middleware (ServeFilesAddon and broccoli-middleware), to rather be a chain of middleware it would be something like:

1. existing middleware
2. watcher middleware: new, ember-cli internal addon named: 'ember-cli:broccoli:watcher"
3. index serving middleware:new, ember-cli internal addon named 'ember-cli:broccoli:serve-files'

Implementation of watcher middleware would build:

 function broccoliMiddleware(request, response, next) {
   watcher.then(function() {
      // do broccoli specific stuff
      request.headers['x-broccoli'] = { 
        outputPath,
        // if we eventually need to add more things
      };
      
       // a good default, downstream can always change this
       response.setHeader('Cache-Control', 'private, max-age=0, must-revalidate')

      // call the next middleware, which by default will be the index serving one
      // but can also be be add-on provided
      next();
   }, function(reason) {
     // unchanged for this feature
   })

Implementation of: index serving middleware (new) would be:

function broccoliServeIndex(request, response, next) {
   // most what is in todays broccoliMiddleware watcher.then(function() { /* this stuff */ };

   buffer = fs.readFileSync(filename)
   response.writeHead(200)
   response.end(buffer)
}

Implementation of an add-on that installs its own middleware

  "name": "fastboot:middleware",
  "keywords": [
    "ember-addon"
  ],
  "ember-addon": {
    "before": "ember-cli:broccoli:serve-files"
  }
}

// index.js
function fastbootMIddleware(request, response, next) {
  if (request.url === request.serveUrl) {
    var fastboot = new FastBoot(data.outputPath);
    fastboot.visit(request.url).then(result => result.html()).then(html => response.end(html)).catch(next);
   }
}

module.exports = {
  serverMiddleware(startOptions) {
    var app = startOptions.app;
    app.use(fastbootMiddleware);
  }
};

This may require other tweaks, but should keep the complexity of the overall system from creeping by ensuring the number of concepts that are different with overlapping goals to a minimum.

This also allows us, to introduce literally no additional add-on API surface area. With the exception, of allowing middleware to be inserted where previously a single middleware incorrectly handled two separate tasks. This prevented the extension point we required.

[stefanpenner]

environment.js is for configuration for our UI code, not for our build. This configuration needs a different place, maybe ember-cli-build, or likely accessible via addon API

[stefanpenner]

We should likely not speculate on how to solve this. Lets define the problem we want to solve, then work together to figure out how to solve. I think this falls into "Unanswered questions"

[kratiahuja]

Agreed. Will move it to "unresolved section".

[stefanpenner]

@rwjblue has some thoughts, we will sync up tomorrow morning, and update our comment based on that :)

[kratiahuja]

@stefanpenner Thanks for the feedback. I will wait for @rwjblue feedback too and then update this RFC.

Here are my two cents (from a naive ember-cli developer who is still learning 😄 ):

1. ember-cli already allows you to provide another middleware rather than using broccoli-middleware. See related code here. I think another approach might be to move setting response headers to serve-files/index.js and just provide another middleware via options.middleware. IMO in this case, there is less friction between ember-cli and broccoli-middleware and then it is clear that broccoli-middleware becomes the default serving file middleware if none is provided.
2. options.middleware is currently only used in testemMiddleware API. How about we extend the same ergonomics for serve task and make it generic?

Thoughts?

[hjdivad]

@kratiahuja there's also https://ember-cli.com/api/classes/Addon.html#method_serverMiddleware which is used more generally (it's the mechanism ServeFiles uses). We want to avoid building out two different APIs for similar functionality.

As I understand it, options.middleware is intended to be used for testing, so I think it makes more sense to use the existing public API (serverMiddleware) than expand the testing option to be more general.

I think another approach might be to move setting response headers to serve-files/index.js and just provide another middleware via options.middleware.

This sounds very similar to splitting up broccoli-middleware in the manner suggested above, ie separating the watcher from the file server. It sounds like we all agree on this point?

[kratiahuja]

@hjdivad I agree we should avoid two different APIs for similar functionality in general.

But here is a usecase that will not work with what Stef is proposing (unless I am misunderstanding something):

1. serve-files middleware that exists today invokes broccoli-middleware already.
2. The way the addon's serverMiddleware's are invoked, it seems that they are invoked before serve-files anyways.
3. We need to have a way to have serve-files or something else to be the last one such that it can set the headers before deciding who should serve.
4. What Stef is proposing to create an addon that calls serverFileMiddleware and broadcasts itself as the last one in the chain (before the serve-files middleware).

If that is the case, who is going to be responsible for setting the correct headers? It is very likely people will end up copying over the setting headers. Atleast in my mind, it seems a lot confusing and lot of friction as to when serverFileMiddleware should be used. If we are saying we have to tweak the existing code, I see it as a major refactor which will cause friction and confusion to addon developers IMO.

If everyone strongly proposes to go down the serverFileMiddlware with probably making me understand my above question and how the above usecase will be solved, I will be happy to update this proposal. If that is going to be the path, then also I don't see what this RFC is trying to expose as public API. I would be happy to close this RFC and just do the PR in that case. 😄

[hjdivad]

@kratiahuja ah okay thank you for clarifying. So you're absolutely right that we missed a piece in our earlier comment, which is that addons (including ones that specify serverMiddleware) can add themselves before or after other named addons. So you wouldn't need to insert yourself at the end, but could instead be "before": "ember-cli:broccoli:serve-files".

See this example in serve-files and the dag code and its call site

[kratiahuja]

@hjdivad Thanks for clarifying! After speaking with you in person I have the clear picture. I will update this RFC by end of today 😄

[rwjblue]

@kratiahuja - I had a conversation with @hjdivad and @stefanpenner about my earlier reservations also, I am fully on board with the suggestion in #80 (review).

[kratiahuja]

@stefanpenner @rwjblue @hjdivad Updated per feedback. I also took the liberty to include the changes in broccoli-middleware to be a utility instead. Let me know your thoughts.

[hjdivad]

Looks great @kratiahuja! I think there's just a small realignment needed per #80 (review)

The only public API advocated in that comment was to have two named middlewares,

• ember-cli:broccoli:watcher
• ember-cli:broccoli:serve-files

And for this to replace serve-files-middleware

This would allow addon authors to be able to insert their own file serving logic via

"ember-addon": {
    "before": "ember-cli:broccoli:serve-files"
  }

without having to deal with the watcher portion and could instead act just like a regular express middleware.

The rest of the comment discussed a starting point for a refactor that would clean up ServeFilesAddon and broccoli-middleware while producing the above API.

As the rfc currently reads to me, its summary is:

1. leave serve-files-middleware in the pipeline
2. add broccoli:serve-files as a second middleware
3. break up broccoli-middleware into some functions.

This doesn't seem to introduce an extension point between the watcher task and the file serving task, which is where the extension point is needed

Can we update the rfc to

1. Be explicit that the only public API change to ember-cli users is the two named middlewares
2. Be clear that that the extension point between those middlewares is between the watcher and the file server (which would still be responsible for setting headers)

That said, the suggested internal changes to broccoli-middleware, ie splitting it up into reusable functions, looks great! 👍

[kratiahuja]

@hjdivad @stefanpenner @rwjblue Updated per feedback.

[stefanpenner]

A further refinement that utilizes the existing middleware ecosystem, and prevents any new concepts from being added would be as follows:

from the point of view of ember-cli, I'm not sure this matters. I believe ember-cli should ask broccoli-middleware for the two middleware. The reason for this, is to ensure we don't add additional concepts, when the existing concepts suffice. In addition, the express middleware ecosystem is what we use, and we should continue to maintain and improve compatibility with the rest of the ecosystem.

it needs:

• watcher middleware
• serve files middelware

watcher middleware

require('broccoli-middleware').forWatcher(watcher);

Implemented something like:

function forWatcher(watcher) {
  return function middleware(request, response, next) {
     // content the same as from: https://github.com/ember-cli/rfcs/pull/80#pullrequestreview-9709454
  }
}

Serve Files

require('broccoli-middleware').serveFiles;

Which is basically as described in: #80 (review)

---

These two middleware can then be inserted in the existing express middleware chain, and if plugin authors wish to augment, they have that extension point.

---

Now, to implement the existing middleware (for backwards compat), we can merely have 1 middleware that in-effect invokes the above 2 in sequence

[kratiahuja]

@stefanpenner Updated per feedback. Please let me know if I missed something.

[stefanpenner]

Looks great!

I really like that we are able to not introduce any new concepts. Instead, largely cleaning up ember-cli to enable this feature.

@hjdivad r? and if you are good, I believe this is good.

This doesn't affect the learning of ember itself, so it will not need ember core teams sign-off.

@ember-cli/core would love some final 👀 on this, but I believe we are looking great.

[stefanpenner]

they look good to me

[hjdivad]

looks great, thanks @kratiahuja!

[stefanpenner]

This is basically good for FCP (final comment period). We missed last weeks meeting by abit, and there was some concern with skipping to FCP before the next ember-cli meeting.

But myself @rwjblue, and @hjdivad feel confident with this approach!

[nathanhammond]

My proposal is that 50% of @ember-cli/core saying that something can proceed to FCP can trigger FCP without it being pulled into the slightly longer "assign, review, (possibly) attach FCP, ship" process core team meeting cadence which makes it at least three weeks of meetings and 15 days. This async approach will likely require working to bring somebody up to speed who is not already familiar with the space–which is a feature, not a bug.

Here is the 50% straw poll. Once we have 50% representation we can move forward and attach FCP. Regardless this is on the agenda for Thursday and reviewing this is part of your homework. 😛

---

• @chadhietala
• @kategengler
• @kellyselden
• @nathanhammond
• @rwjblue
• @stefanpenner
• @trabus
• @Turbo87

[wycats]

My proposal is that 50% of @ember-cli/core saying that something can proceed to FCP can trigger FCP without it being pulled into the slightly longer "assign, review

The Ember process is emphatically not based upon voting, but on consensus. Subteams make progress on RFCs they are responsible for through a consensus of the subteam that the discussion on the RFC has reached a "steady state," and the process is not adding any new information.

In general, once the discussion has reached a steady state, there is broad consensus among the participants about what to do. Consensus does not mean that everyone emphatically agrees with a decision, but rather than none of the participants is prepared to formally object to a proposed disposition.

Occasionally, an RFC reaches a steady state without a broad consensus of all participants. In this case, a consensus of the subteam members can move the process forward. The assumption is that most conversations will progress to a natural broad consensus naturally, and that the fact that the subteam members can resolve steady-state deadlocks will motivate participants to volunteer new information or propose new compromises.

A subteam initiates the FCP process when:

1. there is a consensus among the subteam members that the conversation has reached a steady state
2. there is a consensus among the subteam members about the disposition of the RFC (merge, reject, postpone)

The intent of the FCP process is to solicit new information, so it's quite common for a final comment period to move the conversation back into contention. If this happens, rinse and repeat. If it doesn't, the proposed disposition takes effect.

TL;DR In Ember governance, subteams make decisions by consensus, and the FCP process means that the subteam members have come to consensus that the conversation has reached a steady state and have come to consensus about the proposed disposition.

[wycats]

The major difference between a consensus process like we use in Ember and a voting process is that votes force people with minor (or no) preferences off the fence and give those perspective equal force in the discussion as people with very strong feelings.

A consensus process, by contrast, allows people with strong feelings to object to a particular decision, but creates social norms that:

1. motivate participants to seek new empirical information and compromise
2. encourage thoughtful and weighty objections

There are still some pathologies that a well-run consensus process can produce, and I would recommend watching Brendan Eich's talk Ecma TC39: The Good, The Bad, and The Ugly for an enumeration of some of the things that can go wrong in these processes, which can help members enforce the social norms.

[nathanhammond]

@wycats I don't disagree at all with what you're saying–this proposal is not actually intended to circumvent consensus in any way. It's intended to get us there more rapidly in cases where we're reviewing asynchronously and consider it very likely that we already have consensus. I've attached the full conversation thread from #dev-ember-cli below for the context that was missing in the above note. Reading the thread below you'll see the concerns I voice are exceedingly similar to your own.

The other alternative here takes us three meetings to get through, wherein this makes it possible to address an RFC in just one week for things which we don't believe to be controversial. This whole thing was in order to address the fact that RFCs are often where ideas go to die because of how heavy-handed it is.

---

stefanpenner [9:23 AM]  
@here I would love extra eyes on: https://github.com/ember-cli/rfcs/pull/80 @hjdivad and myself feel that it is good, and any remaining bikeshedding will be merely implementation details to be fleshed out in the PR.  So what I am looking for is any obvious red flags we have missed, but if the “shape” feels right. We should be good

If you had time to review, please leave a :+1: or  :-1: or similar. That way we have some breadcrumbs, rather then just silence. (edited)

kellyselden [9:43 AM]  
That section of cli is a little foreign to me, but I read it and it seems fine to me.

nathanhammond [9:51 AM]  
This is a preliminary review. If this turns up nothing we'll likely put on the agenda as moving to FCP as of next Thursday.

[9:51]  
@rwjblue @stefanpenner you may want to add to the core agenda.

[9:52]  
"Early homework assignment."

rwjblue [10:05 AM]  
@stefanpenner: can we put it in FCP?

nathanhammond [10:05 AM]  
Not until next Thursday.

rwjblue [10:05 AM]  
@stefanpenner: I'd like to review one last time, but won't be able to until Tues

nathanhammond [10:07 AM]  
@rwjblue so you've got time.

rwjblue [10:07 AM]  
Kk

stefanpenner [10:22 AM]  
@nathanhammond I suspect we can FCP it today, if we have some sufficient subset of the ember-cli core team ok with that.

[10:22]  
the only reason i care, is that holidays happen shortly.

[10:22]  
but if we need to defer no big deal.

nathanhammond [10:24 AM]  
I want a conversation amongst the entire team with week-long homework notice so that we're able to confidently move it to FCP.

[10:24]  
We should be sure that we don't have internal conflict before moving it to FCP, that's what this week is for.

[10:24]  
Otherwise we dilute the value of the phrase where it is literally "last call"

stefanpenner [10:25 AM]  
We should also be aware that if some critical mass is already on-board that risk is very very low. e.g. dave rob myself.

[10:25]  
process is process, if that is it then that is ok.

nathanhammond [10:25 AM]  
Agreed, but still important to be careful.

[10:25]  
Our default should be that we core people have zero major feedback during FCP period.

[10:26]  
It can land on Dec 22. :stuck_out_tongue:

[10:26]  
15 move to FCP.

[10:27]  
(basically I don't want churn during the FCP period)

stefanpenner [10:27 AM]  
I think we need to explore another process step in the new year. For some of these things, the current process has an imbalance of scrutiny. So we need to figure that out.

e.g. the final state of this RFC is something we actually we totally consider a no-brainer refactoring, nearly a bugfix. It just took an RFC to to get to that point. (edited)

[10:28]  
What I'm trying to say, is that. I believe we need a “oops this turned into not a new feature, but a small bugfix/refactor”. (edited)

[10:29]  
but once we start down the RFC path, we can’t escape back out :stuck_out_tongue:

rwjblue [10:29 AM]  
@nathanhammond: the point of FCP is to say "we think this is ready for those that have not been paying super close attention to perk up and review".

nathanhammond [10:29 AM]  
Indeed, I'm one of those people.

stefanpenner [10:29 AM]  
but working with community via RFC really really enables us.

nathanhammond [10:29 AM]  
The advantage of the slower and more public process gets much more visibility, and produces more documentation, and helps us understand the tradeoffs.

stefanpenner [10:29 AM]  
i would hate for it to then force extra scrutiny when we can promote it out of that?

nathanhammond [10:30 AM]  
And gets more people involved.

rwjblue [10:30 AM]  
I think FCP based on Stef and David recommendation might be ok

[10:30]  
I don't care a ton either way

stefanpenner [10:30 AM]  
I understand the concern, we need to figure out how to work in this situations as well.

nathanhammond [10:30 AM]  
(By getting buy-in, by getting ownership.)

rwjblue [10:30 AM]  
And I can't review again until tues

stefanpenner [10:30 AM]  
@rwjblue maybe with the criteria of “not a thing that introduces something very different”. (edited)

nathanhammond [10:31 AM]  
That gets hard and I feel uncomfortable having to identify how to make that call.

stefanpenner [10:31 AM]  
i have an easy solution

rwjblue [10:31 AM]  
@nathanhammond: sure, that seems like a reasonable concern

nathanhammond [10:31 AM]  
The _most_ important and valuable thing we've done this year is grow the Ember CLI community.

stefanpenner [10:31 AM]  
Some critical mass of the core team says it can be promoted.

rwjblue [10:31 AM]  
I believe rust does this?

[10:31]  
@locks c/d?

nathanhammond [10:32 AM]  
And I think that some process at service of that goal is infinitely valuable.

stefanpenner [10:32 AM]  
We need RFC so that, we can correctly vet. But we may discover some things require more or less, but today once we go down that path there is no way out.

nathanhammond [10:32 AM]  
I'd do a 50% vote on moving to FCP.

nathanhammond [10:33 AM]  
So that the movement can be done async.

rwjblue [10:33 AM]  
@nathanhammond: ya, seems at least better than unilateral decision

stefanpenner [10:33 AM]  
For me 2-3 subject matter experts is sufficient.

rwjblue [10:33 AM]  
Ok, so ember-cli core team approvals on the PR? (edited)

nathanhammond [10:33 AM]  
50% seems like a good number, you'll be forced to explain it to somebody that _isn't_ a SME.

stefanpenner [10:34 AM]  
50% hurts re network affect.

locks [10:34 AM]  
> I believe rust does this? @locks c/d? - rwjblue

c

stefanpenner [10:34 AM]  
@nathanhammond this is a good conversation, we should continue to iterate

rwjblue [10:34 AM]  
@locks thanks

stefanpenner [10:34 AM]  
i think we have identified the issue

rwjblue [10:34 AM]  
Agreed

stefanpenner [10:34 AM]  
we can spend some time working to solve this one

[10:34]  
so we can all work better :slightly_smiling_face:

locks [10:34 AM]  
I have a bunch of feels about the RFC process, I'd like to put more of it on paper

nathanhammond [10:34 AM]  
Issue is that currently we have no process for moving into FCP other than the one I've been following which requires a homework assignment, a conversation at the core team meeting, and then stamping it with FCP.

locks [10:35 AM]  
so we know what to do in situations like this one

[10:35]  
maybe we form a strike team to come up with a proposal?

[10:36]  
we need a process to move to FCP, we need a process to deprecate RFCs, to close them down (we have so much cruft), etc

nathanhammond [10:36 AM]  
I think we've got a workable proposal.

locks [10:36 AM]  
and tangentially, we should unify the RFC repos

nathanhammond [10:37 AM]  
Async move to FCP requiring 50% of whatever core team to approve. FCP must span a core team meeting for real-time discussion.

[10:37]  
Barring no feedback, FCP can close as expected.

[10:37]  
one week later, still not in lock-step with our meetings.

locks [10:38 AM]  
p.s. I'm still intent on importing Rust's tooling around RFCs for us

[10:38]  
so one way or another this will get codified

rwjblue [10:40 AM]  
@locks when the tooling works *cough* homu *cough*

locks [10:41 AM]  
homu isn't for RFC silly bunny :P

[10:41]  
and their fork works for them

nathanhammond [10:41 AM]  
Note that I see this as an incredibly important goal and I've been playing that role basically all year. I'm less and less the right person as I gain expertise.
 
 > 50% seems like a good number, you'll be forced to explain it to somebody that _isn't_ a SME. - @nathanhammond

[wycats]

@nathanhammond The Rust projects had roughly the same problem and solved it in a very similar way:

1. Instead of waiting for meetings to arrive at consensus, a request for FCP can be made at any time
2. The request includes the checkboxes you included here
3. Once all of the checkboxes have been checked off, the request is approved

Members of the subteam are expected to check the checkbox once they have read the RFC (not to indicate an agreement with the request to enter FCP). Once a member of the subteam has checked their checkbox, they can comment on the thread with a formal concern.

If someone raises a concern, it is attached to the original request.

Here's an example of this process at work in the Rust RFC process: rust-lang/rfcs#1414 (comment)

In this case, the concern was raised at rust-lang/rfcs#1414 (comment)

The Rust project has a pretty nice bot that manages the whole thing that we could use (I've been meaning to suggest that we do so).

The bot also manages a Dashboard: http://rusty-dash.com/fcp. I log into this dashboard periodically to look for any requests-for-consensus bearing my name.

TL;DR the intent is very similar to yours (speed up async decisionmaking and avoid meeting bottlenecks), and assumes consensus. The "checkoff" indicates that a subteam member has read the proposal, and a separate process is used to file objections (to make it clear what exactly your objection is).

The nice thing about meetings is that if you're ever blocked on getting someone to check something off, you can, worst case, bring it to a head at the next meeting, which is no worse than where we are today.

[locks]

Missing word: going to [be]

[kratiahuja]

Fixed...

[kratiahuja]

@nathanhammond @stefanpenner @rwjblue @hjdivad Following up here as well. Is there anything blocking for this RFC to enter FCP? If there is nothing blocking, I would prefer this RFC enters FCP.

[nathanhammond]

After discussion in our last core team meeting we consider this approach to be ideal and that this feature as proposed no longer truly requires an RFC. We see it as changing only private API in a way which approximates a refactor. Given that we've started the RFC process we intend to finish it but we hope to work within the Ember community leadership to come up with something better.

Regardless, this RFC is in final comment period. Barring tremendous concerns we intend to land it and the associated PR.

[kratiahuja]

I plan to send out a PR next week :)

[kellyselden]

@kratiahuja Apologies for the complicated process involved for this RFC. We are actively trying to make it better for the future.

[kratiahuja]

@kellyselden No worries at all. In the bargain, I did get to learn how to write RFCs 😄

[kratiahuja]

Here is the implementation of this RFC:

• broccoli-middleware exposing the correct middleware
• ember-cli creating the proposed two new in repo addons

Plan of action

• broccoli-middleware PR is ready for review
• Once it is merged, release a new version of broccoli-middleware
• Update the version of broccoli-middleware in this PR
• Fix any broken tests in ember-cli and the second PR ready for review
• Update ember-cli-inject-live-reload with updated addon name to depend on and do a release.
• Update changelog.

cc: @stefanpenner @hjdivad @rwjblue

[nathanhammond]

Given that we have received no additional feedback during the FCP period and the team's comfort with the proposed solution (and sketch of the implementation!) we're excited to land this RFC.

Thank you @kratiahuja for shepherding this through the process!