Skip to content
This repository has been archived by the owner on Sep 25, 2024. It is now read-only.

Better patterns for sharing headers/data #1411

Merged
merged 2 commits into from
May 4, 2020

Conversation

ismail-syed
Copy link
Contributor

@ismail-syed ismail-syed commented Apr 30, 2020

Description

Closes #1400

This PR improves the way we add basic data and headers to the React server via Rails. This approach essentially follows whats documented in #1400

  • Patch: Bug/ Documentation fix (non-breaking change which fixes an issue or adds documentation)
  • Minor: New feature (non-breaking change which adds functionality)
  • Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have added a changelog entry, prefixed by the type of change noted above

@ismail-syed ismail-syed force-pushed the quilt_rails-data-support branch 4 times, most recently from 1578b1b to 5eb47e5 Compare April 30, 2020 21:57
@ismail-syed ismail-syed marked this pull request as ready for review April 30, 2020 21:59
@ismail-syed ismail-syed force-pushed the quilt_rails-data-support branch 2 times, most recently from 9580732 to c71e2f0 Compare April 30, 2020 22:15
Copy link
Contributor

@marutypes marutypes left a comment

Choose a reason for hiding this comment

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

A couple notes about the README not matching the API, but otherwise this looks great.

@@ -326,67 +326,42 @@ function App() {
export default App;
```

##### Example: sending headers from Rails controller
##### Example: Sending headers and custom data from Rails controller
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these maybe be two different sections?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yah, they should. Good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried separating these. Lots of redundancy so I just kept them together.


```ruby
class ReactController < ApplicationController
include Quilt::ReactRenderable

def index
render_react(headers: { 'x-custom-header': 'header-value-a' })
render_react(data: { 'x-custom-header': 'header-value-a', 'some_id': 123 })
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think x-custom-header should be passed into data, should it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it shouldn't. This is wrong. In this case, the x-custom-header header would be hidden behind the x-quilt-data header which is not what we'd want for a header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To support the separation of headers/data while ensuring it's easily accessible on the client, I've merged the headers and data in the x-quilt-data response header to the react-server.

@@ -6,29 +6,29 @@ module Quilt
module ReactRenderable
include ReverseProxy::Controller

def render_react(headers: {})
def render_react(headers: {}, data: {})
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the example above should be using headers as well as data

Copy link
Contributor

@janiceblundell janiceblundell left a comment

Choose a reason for hiding this comment

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

Took a look, but nothing to add.

@ismail-syed ismail-syed merged commit ed882c7 into master May 4, 2020
@ismail-syed ismail-syed temporarily deployed to production May 4, 2020 19:33 Inactive
@ismail-syed ismail-syed temporarily deployed to gem May 7, 2020 18:22 Inactive
@BPScott BPScott deleted the quilt_rails-data-support branch May 22, 2021 00:31
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.

Add a way to pass basic data to node via rails, document it's use and places not to use it
3 participants