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

Convert intent.js to TypeScript #185

Merged
merged 37 commits into from
Aug 10, 2020
Merged

Convert intent.js to TypeScript #185

merged 37 commits into from
Aug 10, 2020

Conversation

jaller94
Copy link
Contributor

@jaller94 jaller94 commented Aug 3, 2020

Breaking change:

  • It's no longer ok to hand new Intent(client, client, opts) to Intent and modify opts. Intent will now create a deep clone for itself and ignore outside changes.

@Half-Shot
Copy link
Contributor

Hah, I wrote a branch for this on Friday but never published it. Ah well. This looks very clean.

Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

One thing

src/components/intent.ts Outdated Show resolved Hide resolved
Will Hunt and others added 2 commits August 6, 2020 14:05
@Half-Shot
Copy link
Contributor

@jaller94 do you need any help with these tests?

@jaller94
Copy link
Contributor Author

jaller94 commented Aug 6, 2020

Haven't had time to fix them. I assume

  • I messed up some condition OR
  • type checks became stricter and the test are providing some insufficient mocks.

@Half-Shot
Copy link
Contributor

Might be worth factoring in #155, if you've not already?

@jaller94 jaller94 requested a review from Half-Shot August 7, 2020 19:18
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

One concern

@jaller94 jaller94 self-assigned this Aug 10, 2020
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Further concerns

src/components/intent.ts Outdated Show resolved Hide resolved
src/components/intent.ts Outdated Show resolved Hide resolved
src/components/intent.ts Outdated Show resolved Hide resolved
src/components/intent.ts Outdated Show resolved Hide resolved
src/components/intent.ts Outdated Show resolved Hide resolved
@Half-Shot
Copy link
Contributor

Half-Shot commented Aug 10, 2020

Heads up that I've been trying to incorporate types into matrix-appservice-irc based off this, so I'll have a PR shortly with what I changed to make it work.

const defaultLevel = STATE_EVENT_TYPES.includes(eventType)
? event.content.state_default
: event.content.events_default;
const requiredLevel = event.content.events[eventType] || defaultLevel;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the powerLevelEvent's content did not contain a required level for the eventType and also no defaults, what should happen? Do we just assume the required power level is higher than what we have?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe in theory synapse ensures that should never happen, but we should follow the spec which mentions what values to use if the keys do not exist.

@jaller94 jaller94 requested a review from Half-Shot August 10, 2020 18:58
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Yeah! Happy with this. Thanks for trudging through it

@jaller94 jaller94 merged commit 7df1372 into develop Aug 10, 2020
@jaller94 jaller94 deleted the j94/typescript-intent branch August 11, 2020 08:22
@jaller94 jaller94 removed their assignment Aug 13, 2020
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.

2 participants