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

docs: protocol api: simplification #368

Closed

Conversation

dsanders11
Copy link
Member

@dsanders11 dsanders11 commented Nov 2, 2020

Simplification of the protocol module APIs.

Expose protocol.registerProtocol and protocol.interceptProtocol to allow any type of protocol response, simplifying the API surface of the protocol module. Add several classes to replace the overloaded ProtocolResponse object. Deprecate the current protocol.{intercept|register}{Buffer|File|Http|Stream|String}Protocol methods.

Rendered markdown.

@dsanders11 dsanders11 requested a review from a team as a code owner November 2, 2020 07:59
@nornagon
Copy link
Member

nornagon commented Dec 6, 2022

I like this general proposal but I think we can go further! intercept and register don't need to be two different things; as the only difference between them is that register is for protocols that don't exist yet and intercept is for ones which do, which is something we can determine automatically.

I think we should just have one method, protocol.handle (thanks @dsanders11 for the name suggestion), that can either intercept an existing protocol or register a new one. Multiple handlers registered for the same protocol can chain. For example,

protocol.handle("app", async (request, next) => {
  const response = await getResponseFor(request)
  return response ?? next()
})

Here next would delegate to whatever existing handler there was, or else to some default failure behavior if there was no existing handler.

protocol.handle("http", async (request, next) => {
  if (request.path === '/')
    // I'm unsure if this should be something like "new StringResponse", or whether
    // a plain object is better
    return {content: '<body>hello world', mime: 'text/html'}
  else if (request.path === '/some/image.png')
    return {file: '/path/to/my/image.png'}
  else if (request.path === '/some/video.mp4')
    return {content: generateVideoStream(), mime: 'video/mp4'}
  else if (request.path === '/sneakily-cross-origin')
    return {url: 'https://foo.bar/baz'}
  else return next()
})

@samuelmaddock
Copy link
Member

@nornagon has this design landed?

@dsanders11
Copy link
Member Author

@samuelmaddock, protocol.handle landed which addresses most (all?) of the use cases from this PR, so I'll close this.

@dsanders11 dsanders11 closed this Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants