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

Runtime error when using defaultNodes with SSR #23

Closed
numso opened this issue Aug 7, 2020 · 3 comments · Fixed by #26
Closed

Runtime error when using defaultNodes with SSR #23

numso opened this issue Aug 7, 2020 · 3 comments · Fixed by #26
Labels
bug Something isn't working

Comments

@numso
Copy link
Contributor

numso commented Aug 7, 2020

Versions

Flume: 0.4.0
Browser: Edge 84.0.522.50
Server: Next 9.5.1

Description

When server rendering a flume editor with defaultNodes, the new nodes gets created with a different ID on the server and client

image

This causes a runtime error when creating a connection to that server-rendered node

image

Possible solutions could be

  • have the user provide their own id for each node in defaultNodes (seems like the easiest solution)
  • generate the same id on server/client (can maybe take hints from @reach/auto-id? can't use it directly as it's a hook and we are in a reducer at this point.)
  • do nothing: ssr isn't a goal for this project
  • something else?

I'd be happy to PR something if you let me know which solution path you'd prefer.

Reproduction

Create a next app (yarn create next-app ssr-bug-repro) and add the following page. Then drag a number node on and try to create a connection from it to the default node.

import { NodeEditor, FlumeConfig, Colors, Controls } from 'flume'

const config = new FlumeConfig()
  .addPortType({
    type: 'number',
    name: 'number',
    label: 'Number',
    color: Colors.red,
    controls: [Controls.number({ name: 'number', label: 'Number' })]
  })
  .addNodeType({
    type: 'number',
    label: 'Number',
    description: 'Outputs a numeric value',
    inputs: ports => [ports.number()],
    outputs: ports => [ports.number()]
  })
  .addRootNodeType({
    type: 'homepage',
    label: 'Homepage',
    initialWidth: 170,
    inputs: ports => [
      ports.number({ name: 'copyrightYear', label: 'Copyright Year' })
    ]
  })

export default function Test () {
  // if I uncomment this line below (essentially disabling SSR) everything works fine.
  // if (!global.document) return null
  return (
    <div style={{ width: 800, height: 600 }}>
      <NodeEditor
        portTypes={config.portTypes}
        nodeTypes={config.nodeTypes}
        defaultNodes={[{ type: 'homepage', x: 190, y: -150 }]}
      />
    </div>
  )
}
@chrisjpatty
Copy link
Owner

Thanks for reporting this! It's interesting that server rendering with Next has this problem, but apparently not server rendering with Gatsby, as this problem hasn't surfaced with the server-rendered examples in the docs. It's actually already using @reach/auto-id to manage the IDs of the various stage wrappers, so fully-supporting server rendering is definitely a goal.

I like your idea about providing an id as part of the default nodes, but it does mean the onus is on the developer to ensure that those ids are unique between nodes, and between other instances of the editor that may be on the same page. If it's something that could be abstracted away the way @reach/auto-id does, that would be ideal, but like you say, that's probably a more difficult approach.

One path forward might be to try to detect if a node is being added because it's been server-rendered, and give it a special data attribute if it has. Then when the editor loads in the browser and the default nodes are parsed, it could query the nodes in the stage to see if any of them were server rendered and have the same type as the node being added as a default. If so it could pull the node id from the data attribute of the server-rendered node. Server rendering isn't my forte, so I don't know if there's maybe some potential pitfalls to that approach, but it's maybe a possibility.

What thoughts do you have?

@chrisjpatty chrisjpatty added the bug Something isn't working label Aug 7, 2020
@numso
Copy link
Contributor Author

numso commented Aug 11, 2020

tldr; jump to the last paragraph

I'm pretty new to server-rendering and next myself so I wanted to do some research before I responded to make sure I wasn't using the framework incorrectly. It looks like what next is doing is expected: calling React.hydrate does not clear out existing markup or replace attributes within that markup (sources: facebook/react#10189 and facebook/react#10339)

If you load the flume documentation and check the devtools, you'll see that the default nodes in the flume graph have an ID generated by the server but the client ends up with a different ID. Interestingly, docusaurus does indeed use React.hydrate but never throws warnings in the console. Not sure why it regenerates the ID when next doesn't but my best guess is that docusaurus generates a different structure on the client than on the server so the markup isn't reused (like maybe it's got an extra div surrounding the whole thing)

It looks like React expects that the markup you generate on the server and the markup you generate on your first client render will be identical. So I don't think we can add a data-attribute on the server only because that attribute won't be added in the first client render and we'll get a similar warning about the react output not matching. The way @reach-ui/auto-id handles this is by letting that first server/client render generate undefined as an id and then populating a true id on the second render (source: https://github.com/reach/reach-ui/blob/develop/packages/auto-id/src/index.ts#L80-L107). I don't think this exact approach works for us because each node needs a unique id.

I propose we do something similar to what you suggested: Let's use reproducible IDs for defaultNodes on the first client/server render of flume (maybe id: default-{index}) and give them an extra attribute (something like defaultNode: true). Then on the second render we can replace all those ids with actual ids: useEffect(() => findEveryNodeWithDefaultNodeEqualToTrueAndGenerateANewIdForIt(), []). What do you think?

@chrisjpatty
Copy link
Owner

Interesting, thanks for doing this research. I agree, that sounds like the right approach especially as a first implementation, and it can be revisited again later if needed. Thanks for offering to PR this, if you put one in I'll be sure to get it reviewed and merged in short order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants