airbnb / hypernova

A service for server-side rendering your JavaScript views
MIT License
5.82k stars 212 forks source link

Thoughts on extending the API #59

Closed magicmark closed 7 years ago

magicmark commented 7 years ago

Hi again!

Consider a scenario where inside of getComponent, we'd like to return a React component rendered with either with renderReact or renderReactStatic, depending on the context (eg legacy reasons).

My initial thought is that we could do something funky like define NameOfComponent and NameOfComponentStatic and pass such args though the component name.

What would be even better though is to be able to define custom args; something like

{
  "NameOfComponent": {
    "name": "NameOfComponent",
    "args": {
       "renderType": "static",
    },
    "data": {
      "theseAreProps": true,
      "someOtherProps": ["one", "two", "three"]
    },
  }
}

and args gets passed alongside name to getComponent.

Does this seem reasonable? Thanks!

ljharb commented 7 years ago

That seems like something you could do by defining your own getComponent function and Component.name

ljharb commented 7 years ago

Ah, I've misread; you want to call a different render method based on the name of the component. This definitely seems like something you could do yourself by implementing your own renderReact method - I'm curious what your "legacy reasons" are, but complicating APIs for legacy reasons is generally an unwise idea.

goatslacker commented 7 years ago

Yes this sort of logic is best served by a hypernova-client like hypernova-react.

A hypernova client is just a function so you can do and return whatever you'd like.

magicmark commented 7 years ago

Thanks for the speedy responses!

@goatslacker I might be misunderstanding things here - without changing the component name, I'm not sure how to pass down this flag from the consuming frontend server?

Just to start again to clarify:

I'd like to keep the name of the component (in our case "NameOfComponent") the same, but be able to choose between static/dynamic rendering. There are two ways I can see of doing this:

  1. Changing the name of the component to control this - i.e. define NameOfComponentStatic and NameOfComponentDynamic and use renderReact/renderReactStatic as appropriate, or even a new method that encapsulates this bit of logic as suggested. This was my initial thought, but I'm not sure changing the name of the component just to pass in args seems that great? What are your thoughts on this?
  2. Extending the API to be able to pass extra non-prop args, such as the static/dynamic flag, which gets passed alongside name to getComponent.

(fwiw the legacy reasons are as part of a migration - for shared components that have been ported but shouldn't be react components yet, and so shouldn't be hydrated on the client)

Thanks for being so patient with me on this :)

magicmark commented 7 years ago

@ljharb @goatslacker @spikebrehm Any further thoughts on this?

goatslacker commented 7 years ago

@magicmark

Can this do what you want?

const { renderReactStatic, renderReact }  = require('hypernova-react')

const Component = require('/some/path/to/a/component')
const staticRender = renderReactStatic('NameOfComponent', Component)
const dynamicRender = renderReact('NameOfComponent', Component)

const renderStaticOrDynamic = isStatic => (
  isStatic === true ? staticRender() : dynamicRender()
)

module.exports = renderStaticOrDynamic
magicmark commented 7 years ago

@goatslacker I still need a way to pass the 'isStatic' flag (amongst other server only args) through to Hypernova from the clientlib :)

(The branch I'm working on does this by passing it through the currently undocumented context arg, but it'd be nice to have a proper documented way of doing this that I can merge upstream)

magicmark commented 7 years ago

So we've actually come full circle on this issue

We might have potential collisions in the browser when we do renderReact(name, ...) - if two packages both define a component of the same 'name'. So to prevent this, we're going to namespace our components, which means we'll just go with option 1 and encode all 'server' data in the component.

goatslacker commented 7 years ago

I still need a way to pass the 'isStatic' flag (amongst other server only args) through to Hypernova from the clientlib :)

I see, so your other server is the one that holds the knowledge on whether you want to render something static or non-static. If thats' the case you can pass that sort of information down with the initial payload:

{
  renderStyle: 'static',
  componentProps: serializableProps,
}