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

channels: more metadata work #4239

Open
wants to merge 8 commits into
base: release-channels-updates
Choose a base branch
from

Conversation

arthyn
Copy link
Member

@arthyn arthyn commented Dec 2, 2024

This was a request from @davidisaaclee. We add the new metadata field to the create payload so that we can immediately pass custom channels data when creating a channel. Also adds the types from #4109 into the channels file.

PR Checklist

  • Includes changes to desk files
  • Describes how you tested the PR locally (test ship vs livenet)
  • If a new feature, includes automated tests
  • Comments added anywhere logic may be confusing without context

@arthyn
Copy link
Member Author

arthyn commented Dec 2, 2024

Notably the frontend type changes here need to be fully realized across the codebase before we can deploy.

@davidisaaclee
Copy link
Contributor

davidisaaclee commented Dec 2, 2024

Thanks for this - it looks like my other request from last night got lost: We need an unstructured JSON config dict in each of content renderer / draft input / collection, like this diff: https://gist.github.com/davidisaaclee/097c480318d6a9a8367faaaf8c447360 (there's some more context in our DMs)

I don't understand why Stringified is needed - and if we truly do need to stringify, why can't we do something like JS's JSON.stringify (instead of "_ value" wrapping)? (Not 100% sure that I'm reading this right.)

edit: unless the idea is that the ChannelMetadataV1 type is just a suggestion, and I should just use whatever type I want?

@mikolajpp
Copy link
Collaborator

mikolajpp commented Dec 3, 2024

@arthyn I think there is a problem with state migration in %channels-server. The deployed hooks stuff increased the state version to %7 already, so the type changes in release-channel-updates branch have to go with %8. I have integrated the hooks changes in this branch already, perhaps you can take a look and rebase your changes here against the latest commit.

Edit: sorry about the delay between the last develop merge and the migration fix! There were some huge pretty-printer errors and I only managed to fix this today.

@arthyn
Copy link
Member Author

arthyn commented Dec 3, 2024

@davidisaaclee yeah ChannelMetadataSchemaV1 was just a suggestion based on some old conversations we had, but it can be whatever we want it to be because it's just an open field on the backend. The reason I have stringified here is because the field on the backend is just plaintext or null. So I thought having a more type safe way of doing the json parse/stringify might be nice. we could store it as an actual JSON type hoon-side I just wasn't sure we would want that since it incurs extra structure that we're not really using on the backend.

@davidisaaclee
Copy link
Contributor

davidisaaclee commented Dec 3, 2024

great, I can work with that, as long as the value is the same shape (e.g. an unmodified string) everywhere that API handles the metadata.

maybe we could just change the metadata type to an opaque nominal type (TS hack for this here) - I'm afk now but I can make an inline suggestion comment later

@mikolajpp mikolajpp force-pushed the release-channels-updates branch 3 times, most recently from 802a1eb to 8fafff9 Compare December 5, 2024 05:51
Copy link
Collaborator

@mikolajpp mikolajpp left a comment

Choose a reason for hiding this comment

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

The Hoon part looks good. Typescript part should have another look by someone else to be sure. Thanks!

One question: is adjusting the channel metadata after creation not in the picture? I can only see it supplied in %create poke. Do we need a %meta command in $c-channel as well?

@arthyn
Copy link
Member Author

arthyn commented Dec 9, 2024

@mikolajpp that was already there in the original meta PR, we just missed creation so I added it here

@davidisaaclee
Copy link
Contributor

talked with Hunter offline, TS looks good – here's the change I mentioned in my comment but it's going to be messy to propose as a commit to this and shouldn't block, so I'll just fix forward once this PR is merged

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.

3 participants