Skip to content
This repository was archived by the owner on Nov 18, 2022. It is now read-only.

Feature helpers ts - #1

Merged
darh merged 20 commits into
developfrom
feature-helpers-ts
Feb 12, 2020
Merged

Feature helpers ts#1
darh merged 20 commits into
developfrom
feature-helpers-ts

Conversation

@Fajfa

@Fajfa Fajfa commented Feb 4, 2020

Copy link
Copy Markdown
Member

Files to be converted to Typescript

  • Shared
  • System
  • Messaging
  • Compose

@Fajfa
Fajfa requested a review from darh February 4, 2020 12:00

@darh darh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This needs some love.
Plus -- please spend some time studying existing code/functions in this repo (like casters, guards... ) and how they are used.

Comment thread src/corredor/helpers/shared.ts Outdated
Comment thread src/corredor/helpers/shared.ts Outdated
Comment thread src/corredor/helpers/shared.ts Outdated
@darh darh self-assigned this Feb 6, 2020
@Fajfa
Fajfa force-pushed the feature-helpers-ts branch from 7fb622e to 3f35f20 Compare February 6, 2020 15:10

@darh darh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good first step, but let's improve it as much as we can.

Please take a look at tsdoc and how can existing (jsdoc) comments be changed

Comment thread src/corredor/helpers/shared.ts Outdated
Comment thread src/corredor/helpers/shared.ts Outdated
Comment thread src/corredor/helpers/system.ts Outdated
Comment thread src/corredor/helpers/messaging.ts Outdated
Comment thread src/corredor/helpers/messaging.ts Outdated
Comment thread src/corredor/helpers/messaging.ts Outdated
Comment thread src/corredor/helpers/system.ts Outdated
*/
export class System {
constructor (ctx = {}) {
private SystemAPI: SystemAPI;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This MUST be set, not optional.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(same in other helpers)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Should only the API be set or all the ctx params ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

inspect the code and propose/decide :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The API and namespace params are required. Everything else is optional.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Scratch that, namespace is optional too.

Comment thread src/corredor/helpers/system.ts Outdated
Comment thread src/corredor/helpers/messaging.ts Outdated
Comment thread src/corredor/helpers/messaging.ts Outdated
private $channel?: Channel;
private $message?: Message;

constructor (ctx: Context) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You could replace Context with Partial<Messaging>
https://www.typescriptlang.org/docs/handbook/utility-types.html#partialt

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've tried to do this, but i can't get it working. Looked at other code using Partial and tried it that way. No luck.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What did you try and what does not work?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Partial sets all the props to optional, so that isn't too good since we want the API to be set.

@Fajfa
Fajfa force-pushed the feature-helpers-ts branch from 3f35f20 to a9bd7c4 Compare February 9, 2020 23:39
@Fajfa Fajfa changed the title WIP: Feature helpers ts Feature helpers ts Feb 10, 2020
@Fajfa
Fajfa force-pushed the feature-helpers-ts branch from 434dc30 to 1306840 Compare February 10, 2020 12:32
@Fajfa

Fajfa commented Feb 10, 2020

Copy link
Copy Markdown
Member Author

Theres still an error in messaging.ts helper on line 85. Casting to new Message() doesn't yet work, because the type isnt done yet. The line is commented for now.

@Fajfa
Fajfa force-pushed the feature-helpers-ts branch from 1306840 to c893893 Compare February 10, 2020 12:54
Comment thread src/corredor/helpers/messaging.ts Outdated
}

interface ChannelResponse {
[_: string]: unknown;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this [string]:unknown really needed?

channel response (as well as any other list responses) are always composed of filter+set.
No other arbitrary props.

(applies to other cases here too)

@Fajfa Fajfa Feb 11, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

the [_:string]: unknown in all the responses is there so i dont have to convert from API response to unknown to Channel Response(res as unknown as ChannelResponse). I can instead just do:
res as ChannelResponse

Comment thread src/corredor/helpers/messaging.ts Outdated
Comment thread src/corredor/helpers/shared.ts Outdated
Comment thread src/corredor/helpers/compose.ts Outdated
Comment thread src/corredor/helpers/compose.ts Outdated
Comment thread src/corredor/helpers/compose.ts Outdated
Comment thread src/corredor/helpers/messaging.ts Outdated
private $channel?: Channel;
private $message?: Message;

constructor (ctx: Context) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What did you try and what does not work?

@darh darh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

almost there.

@Fajfa
Fajfa force-pushed the feature-helpers-ts branch from c893893 to 4ef8ed8 Compare February 11, 2020 12:21
@darh
darh force-pushed the feature-helpers-ts branch from 4ef8ed8 to 8d55f7f Compare February 11, 2020 18:15
@darh
darh merged commit 3a9c5c6 into develop Feb 12, 2020
@darh
darh deleted the feature-helpers-ts branch February 12, 2020 06:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants