Skip to content

New discord.js-react utility bridge library#44

Draft
dominnya wants to merge 5 commits intoitsMapleLeaf:mainfrom
dominnya:djs-bridge
Draft

New discord.js-react utility bridge library#44
dominnya wants to merge 5 commits intoitsMapleLeaf:mainfrom
dominnya:djs-bridge

Conversation

@dominnya
Copy link
Copy Markdown
Collaborator

@dominnya dominnya commented Nov 4, 2023

  • Resolves useReactions() hook #33
  • Documentation README.md
  • useMessage hook to return instance message
  • Manual tests for each hook
  • Unit tests

Note
Additionally implemented bounce counter test in order to test useEffect

@dominnya dominnya added the enhancement New feature or request label Nov 4, 2023
@dominnya dominnya requested a review from itsMapleLeaf November 4, 2023 17:41
@dominnya dominnya self-assigned this Nov 4, 2023
@dominnya
Copy link
Copy Markdown
Collaborator Author

dominnya commented Nov 4, 2023

Implementing useMessage would require a bit of context rewrite as far as I can see

Comment thread packages/discord.js/library/use-reactions.ts Outdated
Comment thread packages/discord.js/library/use-reactions.ts
Comment thread packages/discord.js/library/use-reactions.ts Outdated
Comment thread packages/discord.js/library/use-reactions.ts Outdated
Comment thread packages/discord.js/library/use-reactions.ts Outdated
return () => collector.stop()
}, [message, update])

return { reactions, alive, collector }
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Is exposing the collector useful here? That feels like an implementation detail that could change over time. If the use case is wanting to be able to control the behavior, I'd expose wrapped callbacks or additional options instead (like an active option)

I also don't see a use case for alive - I feel most usages of this hook only care about the current reactions

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The alive use case should've been an addition for watching the effect. Yes, I should definitely wrap callbacks. I will look into the implementation later once I solve the message context problem.

Comment thread packages/discord.js/package.json Outdated
Comment on lines +3 to +5
export function useMessage(): Message {
return {} as Message;
} No newline at end of file
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

To make this work, the DJS adapter could render its own context provider with whatever content the user passes:

// reacord-discord-js.tsx
export const MessageContext = createContext<Message | undefined>(undefined)

export class ReacordDiscordJs extends Reacord {
	// ...

	protected createInstance(renderer: Renderer): ReacordInstance {
		const { render, ...instance } = super.createInstance(renderer)
		return {
			...instance,
			render(content) {
				return render(
					// renderer.message needs to be public now
					<MessageContext.Provider value={renderer.message}>
						{content}
					</MessageContext.Provider>,
				)
			},
		}
	}

	// ...
}

Although 🤔 since this is a separate package, that means MessageContext has to become a public export of the library... not great.

I guess we could just mark it as @private in JSDoc, but the "real" solution would be to move ReacordDiscordJs to this new package, so it can use that context without having to export it publicly

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes I was thinking about a similar solution, this is why I left it empty. I will investigate the problem further. Thank you for an example

Co-authored-by: Darius <19603573+itsMapleLeaf@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

useReactions() hook

2 participants