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.

Sign up for GitHub

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

Jump to bottom

realtime persistence. #1952

Merged
merged 7 commits into from
Dec 2, 2021
Merged

realtime persistence. #1952

merged 7 commits into from
Dec 2, 2021

Conversation

0xc0Der
Copy link
Contributor

@0xc0Der 0xc0Der commented Nov 15, 2021 •
edited

realtime persistence: solves issue #1196.

  • websocket.
  • socketio.
  • sse.
  • mqtt.

@netlify
Copy link

netlify bot commented Nov 15, 2021 •
edited

‼️ Deploy request for hoppscotch rejected.
Learn more about Netlify's sensitive variable policy

🔨 Explore the source changes: 5363b2b

@0xc0Der 0xc0Der changed the title [WIP] realtime persistence. realtime persistence. Nov 17, 2021
@0xc0Der 0xc0Der marked this pull request as ready for review November 17, 2021 09:59
AndrewBastin
AndrewBastin requested changes Nov 17, 2021
View reviewed changes
Copy link
Member

@AndrewBastin AndrewBastin left a comment •
edited

Choose a reason for hiding this comment

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

Please do note the comments regarding the TypeScript type definitions. Although many components in Hoppscotch are not still TypeScript, we are moving towards a complete TypeScript system to ensure a more stable Hoppscotch experience both for the users and for developers of Hoppscotch.

Please make sure your PRs have atleast valid code before submitting it (or mark it as a draft PR). I will try patching these myself when I get time myself but these type of behaviour blocks other things in Hoppscotch us maintainers are working on.

Thank you.

@liyasthomas can you update the Contributors guide or CoC to emphasize this ?

@0xc0Der
Copy link
Contributor Author

0xc0Der commented Nov 17, 2021 •
edited

sorry about that. for socket i did not really know what type to use, and I preserved it to stay open when switching tabs.
the others are minor fixes I will do them my self.

@AndrewBastin
Copy link
Member

Cool, please do the needful, marking this PR as Draft meanwhile.

@AndrewBastin AndrewBastin marked this pull request as draft November 19, 2021 00:24
@0xc0Der
Copy link
Contributor Author

0xc0Der commented Nov 20, 2021

can you take a look at this.
also,
NOTE: I found a strange behavior with typescript that it doesn't care anymore about types, it doesn't complain or raise any errors.
I don't know if it's just with me or it is a bug. Hope you take a look at it.

@AndrewBastin
Copy link
Member

I will look into the PR when I get time.

About the TypeScript thingy, our current typescript checking is kind of restricted only to the IDE without any compile time checks. We are still working out some kinks with our code and TypeScript validation passing before enforcing it.

Thank you!

@AndrewBastin AndrewBastin marked this pull request as ready for review November 22, 2021 07:31
AndrewBastin
AndrewBastin requested changes Nov 30, 2021
View reviewed changes
Copy link
Member

@AndrewBastin AndrewBastin left a comment

Choose a reason for hiding this comment

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

Heyyoo @0xc0Der !

Sorry for the delay, got caught up with other features and stuff.

I have added some more change requests, please look into them.

Just lemme know if you can't do it, I will implement it myself then.

packages/hoppscotch-app/newstore/MQTTSession.ts Show resolved Hide resolved
packages/hoppscotch-app/newstore/MQTTSession.ts Outdated Show resolved Hide resolved
packages/hoppscotch-app/newstore/SocketIOSession.ts Show resolved Hide resolved
packages/hoppscotch-app/newstore/SocketIOSession.ts Outdated Show resolved Hide resolved
packages/hoppscotch-app/newstore/SSESession.ts Show resolved Hide resolved
packages/hoppscotch-app/newstore/SSESession.ts Outdated Show resolved Hide resolved
packages/hoppscotch-app/newstore/WebSocketSession.ts Show resolved Hide resolved
packages/hoppscotch-app/newstore/WebSocketSession.ts Outdated Show resolved Hide resolved
@AndrewBastin AndrewBastin self-requested a review December 1, 2021 17:46
@AndrewBastin
Copy link
Member

Thank you for the PR @0xc0Der !

Sorry for the delay.

Merging!

@AndrewBastin AndrewBastin merged commit cc81242 into hoppscotch:main Dec 2, 2021
@liyasthomas liyasthomas mentioned this pull request Dec 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndrewBastin AndrewBastin AndrewBastin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@0xc0Der @AndrewBastin