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

New discord.js-react utility bridge library #44

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

domin-mnd
Copy link
Collaborator

  • 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

@domin-mnd
Copy link
Collaborator Author

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

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

return { reactions, alive, collector }
Copy link
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
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.

packages/discord.js/package.json Outdated Show resolved Hide resolved
Comment on lines +3 to +5
export function useMessage(): Message {
return {} as Message;
}
Copy link
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
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

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