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

useBootstrap hook #102

Closed
wants to merge 16 commits into from
Closed

useBootstrap hook #102

wants to merge 16 commits into from

Conversation

neftaly
Copy link
Collaborator

@neftaly neftaly commented Jun 25, 2023

This is an experimental hook for getting the current documentId from the hashroute URL, and creating a new document if one is not set.

const App = ({ userId}) => {
  const documentId = useDocumentId(
    (s: any) => {
      // Set initial state if a new document was created
      s.layers = ["bun", "lettuce", "patty", "bun"];
    }
  )
  const [doc, changeDoc] = useDocument(documentId);

My goal is to remove as much document setup boilerplate as possible from the app entrypoint (i.e. main.jsx), and to support hashroute changes.

For example, we'd remove most of this code from automerge-repo-demo-todo/src/main.tsx:

const getHashValue = (key: string) => {
  const { hash } = window.location
  var matches = hash.match(new RegExp(`${key}=([^&]*)`))
  return matches ? matches[1] : undefined
}

const getRootId = () => {
  const idFromHash = getHashValue("id")
  if (idFromHash) return idFromHash as DocumentId

  // create an empty document
  const handle = repo.create<State>()
  // set its initial state
  handle.change(s => {
    s.todos = []
  })
  return handle.documentId
}

const rootId = getRootId()
window.location.hash = `id=${rootId}`

ReactDOM.createRoot(document.getElementById("root") as HTMLElement).render(
  <RepoContext.Provider value={repo}>
    <React.StrictMode>
      <App rootId={rootId} />

Instead of <App> taking a rootId prop, we'd just use the hook (with an additional onCreate argument for setting initial state, as well as a getter/setter for custom hash routing).

@neftaly
Copy link
Collaborator Author

neftaly commented Jun 25, 2023

You can either join an existing documentID by hashroute, or create a new one (by not having any hashroute).
It does not currently support joining a non-existant documentId.

A demo is available at https://github.com/neftaly/autoburger

@neftaly
Copy link
Collaborator Author

neftaly commented Jun 25, 2023

I don't like how the "onCreate" function ties into getting and setting document ID strings. I'm not yet sure how else to integrate it into the system, however.

@pvh
Copy link
Member

pvh commented Jun 27, 2023

I really like the goal of this PR but I have a few review comments:

  • I try to avoid introducing dependencies to automerge-repo. It looks like the one hook you're requiring is pretty straightforward, can we just vendor that somewhere? I could be persuaded it was worth adopting if we were going to use it a lot but the library looks a bit like a sort of "party repo" which merges lots of different ideas and I'm not sure it'll all be of the same quality.
  • useDocumentId() is too generic a name for what you're doing here. It should reference the hash, right?
  • should this include code for storing the documentId in localStorage? I don't think we want going to a blank URL to always create a new document.

Overall though, I think something like this is a really good idea! The bootstrapping process is an important one to get right and the demo code currently does not :)

@neftaly
Copy link
Collaborator Author

neftaly commented Jun 28, 2023

Thanks for your comments. The behaviour of useHash can absolutely be internalized, I'm only using half of it anyway.

I'm not yet sure what the right approach to bootstrapping is, so I don't actually know what to name this yet - I think that will come last :)

I would like to separate hashroute behaviour from document setup, though I'm not yet sure how to then connect the two. The document setup could take a function for various setup strategies (checking localstorage etc). I would also like to consider basic hashroute interop with routing libraries.

@pvh
Copy link
Member

pvh commented Jun 28, 2023

Let's see -- can we agree on the pattern here? You have a documentId of some kind in a queryParam, or if it's not present, you might have it in a data store, and if not that, we make one.

This feels like a cascading set of options that might read something like

const bootstrap = (source, initFunction) => 
  useQueryParam(source) 
  || useSavedDocumentId(source)
  || useNewInitializedDocument(initFunction)

const docId = bootstrap("document", initFunction)
const handle = repo.find(docId)

What I like about this approach is that it reads clearly but exposes what's happening. If this, then that. I could be persuaded by something else but that's at least a straw-man proposal to keep the conversation going. What do you think?

@neftaly
Copy link
Collaborator Author

neftaly commented Jun 29, 2023

This sounds like a good strategy, I will follow up on it. Hooks won't work exactly like this but I like the idea of a simple composition you could edit for your own needs.

I would like to consider how a physically local document ID source could tie into this, for example from a bluetooth beacon or mDNS advertisement, however this is not a significant concern.

@pvh
Copy link
Member

pvh commented Jul 1, 2023

Looking forward to the patch!

@neftaly
Copy link
Collaborator Author

neftaly commented Jul 4, 2023

This commit internalizes useHash. It also (optionally) looks up document IDs in localStorage, if a hashroute ID is unavailable.

If a document is already loaded, but you want to create a new one:

const createNewDocument = (repo, onCreate) => {
  const { documentId } = createDocument(repo, onCreate)
  setHash(documentId)
}

I feel like this is simple enough to be included in the docs, and not part of the hook.

// Update hashroute & localStorage on changes
useEffect(() => setDocumentId(handle.documentId), [hash, handle.documentId]);

// TODO: Should we return a handle, not a documentID?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is a better strategy but want to check first

@neftaly neftaly changed the title useDocumentId hook useBootstrap hook Jul 5, 2023
return existingDocumentId
? repo.find(existingDocumentId)
: createDocument(repo, onCreate);
} catch (e) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm assuming repo.find will throw on invalid document ID

Copy link
Member

Choose a reason for hiding this comment

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

it will ... soon!

@neftaly
Copy link
Collaborator Author

neftaly commented Jul 12, 2023

Merged 1d118fd

@neftaly neftaly closed this Jul 12, 2023
@neftaly neftaly mentioned this pull request Aug 3, 2023
12 tasks
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

2 participants