This repository has been archived by the owner on Jan 30, 2025. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Initial surface for module API (enough for an ILAG module) #1
Initial surface for module API (enough for an ILAG module) #1
Changes from 5 commits
de97ef7
7446412
f47cdd4
e294adf
15b71d4
cf43dff
3f60091
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't this just grow a peer-dep to js-sdk given it knows it'll have a js-sdk from react-sdk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially. I've been trying to avoid the peer dependency style as it'll end up being a slew of warnings for developers (because we won't update the dependency very often)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's fine though because its a peer-dep, you could even specify the min version and say any version from there.
>=12.4.1
(replacing with the actual version we introduced TypedEventEmitter)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
until we release a breaking change of the js-sdk and then it is considered unmatched (both logically and by the semver checks - we can't assume it'll continue to compile).
For now I'm not that concerned with typed event emitter support. Documentation can supplement it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then why not add yet another layer called
matrix-js-common
or something :P?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if serious, but it's quite literally on the table for consideration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be a good home for some things for element-desktop to re-use too, like
fetchdep.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably for a larger conversation outside of this PR though ;)