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

refactoring #26

Merged
merged 4 commits into from
Mar 15, 2019
Merged

refactoring #26

merged 4 commits into from
Mar 15, 2019

Conversation

santakadev
Copy link
Contributor

@santakadev santakadev commented Mar 3, 2019

Why refactor codebase?

Right now there are too many responsabilities and IO (storage, editor, network and DOM) in the same module. This makes testing really hard and it is not clear what the application is doing.
The idea behind this PR is split this module and create some abstractions to make testing and future development easier.

For all that, I think that this codebase refactor is a must before start milestone 1.1 development.

What I have done?

Two things:

  • Refactoring: cleanup and add new abstractions
  • ES6: setup babel and use ES6 syntax

Added abstractions:

  • P2PFile. Represents the storage of a source code file via deltas. Coupled to hypercore and simple-peer for replication, but decoupled from webrtc-swarm, signalhub and ace. This abstraction can help us in the future when dealing with multiple files.
  • Editor. Wrapper of ACE editor.

Aditional concepts added:

  • session: represents the current coding session
  • sessionId: id of the session.
  • follower: represents the peers that are following the session (read only)

Things I'm not sure about

Add Net abstraction

I've the tentation of making another abstraction called Net, that holds the webrtc connection via signalhub and webrtc-swarm. But for now, I think that I can leave it as it is, and reconsider it when implementing #9 Use UUIDs as session IDs and #7 Connected users list

Where to put browser IO (console, window)

I'm not happy about having URL sessionId extration/addition outside P2PEditor class. Currently is in index.js file, but I think that it must belong to P2PEditor class.

I have done this to have the browser IO (window and console) in another module. This is not enterely true, beacuse ACE is doing browser IO.

The two alternatives that I have in mind:

  • Crate another abstraction called Session (current P2PEditor class), and put all the index.js code in a class called P2PEditor. In this case, P2PEditor will be holder of the session and will make browser IO based on the Session dispatched events. But I think that session is something that never changes until you exit the window. I don't imagine a feature in witch I switch to another sessión with other peers and file without leaving the browser window.
  • Move all the logic to current P2PEditor. This way all the logic is inside P2PEditor class, but without introducing another abstraction.

@santakadev santakadev self-assigned this Mar 3, 2019
@santakadev santakadev force-pushed the refactoring branch 2 times, most recently from 54a17cd to 80ba50b Compare March 4, 2019 05:44
@santakadev santakadev changed the title refactoring and testing before 1.0 tag refactoring Mar 7, 2019
@santakadev santakadev marked this pull request as ready for review March 7, 2019 06:14
Copy link
Member

@JavierCane JavierCane left a comment

Choose a reason for hiding this comment

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

After reviewing the PR, as you suggested in the PR description, I would also go ahead in a further refactor (in a different PR) extracting the Session concept to its own class in order to manage the webrtcSwarm connection and the underlying signalhub.

This way we would be leaving the P2PEditor class with the bare minimum business logic. It would act just as an orchestrator bewteen the differen pieces (Session, Editor, and ChangeLog).

Changes suggested in order to keep a minimum structure while firing events. Very good job @santakadev! 🙌

src/p2p-editor.js Outdated Show resolved Hide resolved
src/p2p-file.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/editor.js Outdated Show resolved Hide resolved
src/p2p-editor.js Outdated Show resolved Hide resolved
src/p2p-file.js Outdated Show resolved Hide resolved
src/p2p-editor.js Outdated Show resolved Hide resolved
src/p2p-editor.js Outdated Show resolved Hide resolved
src/p2p-editor.js Outdated Show resolved Hide resolved
src/p2p-editor.js Outdated Show resolved Hide resolved
@santakadev
Copy link
Contributor Author

Thank you @JavierCane for your feedback!

I'll apply your suggested naming improvements and merge this PR to master.

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.

None yet

2 participants