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

Protocol Integration #35

Merged
merged 32 commits into from
Feb 2, 2023
Merged

Protocol Integration #35

merged 32 commits into from
Feb 2, 2023

Conversation

jackyzha0
Copy link
Contributor

@jackyzha0 jackyzha0 commented Jan 16, 2023

Closes hyphacoop/distributed-press-organizing#62

  • API calls now hooks up with appropriate protocols based on config

@jackyzha0 jackyzha0 requested a review from RangerMauve January 16, 2023 00:55
Copy link
Contributor

@RangerMauve RangerMauve left a comment

Choose a reason for hiding this comment

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

Awesome! I like where this is going. Some minor nits but I think this is a better structure from before. sync vs unsync is good wording for it IMO.

@fauno Heads up, this changes the HTTP API a bit for updating site data.

v1/api/sites.ts Outdated Show resolved Hide resolved
v1/config/sites.ts Outdated Show resolved Hide resolved
v1/fs/index.ts Outdated Show resolved Hide resolved
@fauno
Copy link
Collaborator

fauno commented Jan 17, 2023

@fauno Heads up, this changes the HTTP API a bit for updating site data.

Could you point me to those changes? I saw there's a change on NewSite and publication changed to a protocolos object of protocol: Boolean pairs?

v1/config/sites.test.ts Outdated Show resolved Hide resolved
@fauno
Copy link
Collaborator

fauno commented Jan 17, 2023

Could you point me to those changes? I saw there's a change on NewSite and publication changed to a protocolos object of protocol: Boolean pairs?

Nevermind, I'm reading the files :)

@jackyzha0 jackyzha0 marked this pull request as ready for review January 23, 2023 20:39
@fauno
Copy link
Collaborator

fauno commented Jan 26, 2023

i'm having this tsc error when running dev script after the last commit, removing the whole block of sync code con v1/api/sites.ts works for the tests i was doing.

config/sites.ts(34,13): error TS7053: Element implicitly has an 'any' type because expression of type 'string' can't be used to index ty
pe '{ ipfs: boolean; hyper: boolean; http: boolean; }'.             
  No index signature with a parameter of type 'string' was found on type '{ ipfs: boolean; hyper: boolean; http: boolean; }'.
config/sites.ts(35,27): error TS7053: Element implicitly has an 'any' type because expression of type 'string' can't be used to index ty
pe 'ProtocolManager'.                                                                                                                   
  No index signature with a parameter of type 'string' was found on type 'ProtocolManager'.
config/sites.ts(37,20): error TS7006: Parameter 'protocolLinks' implicitly has an 'any' type.
config/sites.ts(38,15): error TS7053: Element implicitly has an 'any' type because expression of type 'string' can't be used to index ty
pe 'Partial<{ ipfs: { gateway: string; cid: string; pubKey: string; } & { link: string; enabled: boolean; }; hyper: { gateway: string; r
aw: string; } & { link: string; enabled: boolean; }; http: {} & { link: string; enabled: boolean; }; }>'.
  No index signature with a parameter of type 'string' was found on type 'Partial<{ ipfs: { gateway: string; cid: string; pubKey: string
; } & { link: string; enabled: boolean; }; hyper: { gateway: string; raw: string; } & { link: string; enabled: boolean; }; http: {} & { 
link: string; enabled: boolean; }; }>'.

not sure if this change affects the api endpoints, i'd have to adapt the client. it seems to work for a nanoid still, but not sure if internally it thinks it's the domain name

-    const id = nanoid()                                                                                                                
+    const id = cfg.domain                                                                                                              

@RangerMauve
Copy link
Contributor

Oh, I messed up some typescript types, working on fixing it now

@RangerMauve
Copy link
Contributor

Just gonna add some more fixes for the protocol stuff first

@RangerMauve
Copy link
Contributor

Make sure to do an npm install call after pulling.

@RangerMauve
Copy link
Contributor

RangerMauve commented Jan 26, 2023

Remaining steps:

Copy link
Contributor

@RangerMauve RangerMauve left a comment

Choose a reason for hiding this comment

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

Other than the one nit pic I think this is good to go!

Mind addressing the comment and then merging in?

Also, I really like your approach of adding a context to tests for setup and teardown.

const obj = {
const id = cfg.domain
if (!isValidHostname(id)) {
return await Promise.reject(new Error('Invalid hostname. Please ensure you leave out the port and protocol specifiers (e.g. no https://)'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does throw not work for this case? If it's a linter thing, I'd appreciate it if we could disable whatever rule is mandating this code style. :P

@jackyzha0 jackyzha0 merged commit 986aae5 into v1-staging Feb 2, 2023
@jackyzha0 jackyzha0 deleted the protocol-integration branch February 2, 2023 21:46
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.

3 participants