[Atom] Create Atom Plugin #93
Conversation
| @@ -0,0 +1,15 @@ | |||
| module.exports = { | |||
There was a problem hiding this comment.
I think we should keep the styling for all our .js files the same and all our .py files the same, and so on - I believe the crdt styling doesn't change any rules from airbnb's default
There was a problem hiding this comment.
Sure. Some of these should be here, like the globals, since they are atom specific.
Others like the rules can be moved to a global config if you're okay with that:
no-underscore-danglemeans you can't have members likethis._privateMemberwhich I am against since thats a convention we follow in the python library as well and is generally well accepted javascriptno-plusplusseems reasonable to have,value++is common and unless you have a good reason why we shouldn't allow it I don't see why not- 'no-linely-if
it should be okay to have anifinside anelse` statement. I think the linter is giving a false positive for me, but even if it isn't Personally I don't see why not.
I'm okay for removing any of these configs. as long as they are given/within reason. But I agree that they should be kept consistent, and we should leave them configs in the tandem root.
There was a problem hiding this comment.
an if inside an else is just an else if isn't it? For the others, sure -- but I think we should move the.eslintrc in the Tandem root folder and symlink it to both the crdt folder and here
| _shutDownAgent() { | ||
| this._subscriptions.dispose(); | ||
| this._processingMessage = false; | ||
| this._agent.kill('SIGKILL'); |
There was a problem hiding this comment.
I think we should default to SIGTERM here so that the agent can shut down gracefully. If we use SIGKILL the agent won't get a chance to inform the other peers that it's leaving the session.
There was a problem hiding this comment.
👍 hopefully this still kills the agent - I recall having issues using SIGTERM which is why I switched to SIGKILL.
| _readAgent(data) { | ||
| // Prevent future 'data' callbacks from being invoked until stream is | ||
| // unpaused | ||
| this._agent.stdout.pause(); |
There was a problem hiding this comment.
Does not having this cause problems? JS should be single threaded so if you're running the code here other callbacks can't be invoked.
There was a problem hiding this comment.
JS is single threaded, but can't the execution jump between two instances of the callback? There's no guarantee that the entirety of readAgent will be invoked before another callback is handled (if, say, two chunks of data are submitted at once) is there? If there is, then this can be removed.
There was a problem hiding this comment.
I doubt it would switch because then you'd be introducing concurrency, ultimately meaning you'd need concurrency control to protect your data.
According to the MDN, the event loop runs functions to completion (i.e. no preemption)
https://developer.mozilla.org/en-US/docs/Web/JavaScript/EventLoop#Event_loop
| console.log('Inavlid message. Expected WriteRequest.'); | ||
| } else { | ||
| // Prevent non-Apply Patches messages from being processed | ||
| this._processingMessage = true; |
There was a problem hiding this comment.
Even if we don't send NewPatches while this flag is up the user can still edit the text buffer, which can cause inconsistencies. Is there no other way around this?
In the worst case I nothing can be done I think we should actually go as far as undoing the user's change if there is one while this flag is up. It should be rare, but if an inconsistency actually does happen it may crash the CRDT.
There was a problem hiding this comment.
There's no way to stop the user from editing the buffer. We'll have to undo the change.
| const message = m.deserialize(data); | ||
|
|
||
| if (!this._processingMessage) { | ||
| if (!(message instanceof m.WriteRequest)) { |
There was a problem hiding this comment.
With the hole punching changes the agent may also send the SessionInfo message. This is typically only sent once though.
| // Prevent future 'data' callbacks from being invoked until stream is | ||
| // unpaused | ||
| this._agent.stdout.pause(); | ||
| const message = m.deserialize(data); |
There was a problem hiding this comment.
This assumes that a data event contains a complete message. I'm not sure if that's guaranteed by node, especially if it's a large message. I think to be safe we should be looking for the newline character to know if we've reached a message boundary. We do something similar when processing messages on the CRDT process.
https://github.com/geoffxy/tandem/blob/master/crdt/io/index.js#L7
| const getStringPort = () => | ||
| String(random(60600, 62600)); | ||
|
|
||
| const createPatches = event => |
There was a problem hiding this comment.
How does teletype handle this? If possible we want to avoid having to apply changes in two steps since we can get weird interleaving.
| "tandem:leave-session" | ||
| ] | ||
| }, | ||
| "repository": "https://github.com/geoffxy/tandem", |
There was a problem hiding this comment.
If we end up open sourcing this I think we should move the repo under typeintandem. I think we should just omit this for now.
There was a problem hiding this comment.
Agreed, I'm fine with that.
| ] | ||
| }, | ||
| "repository": "https://github.com/geoffxy/tandem", | ||
| "license": "MIT", |
There was a problem hiding this comment.
Maybe also omit this for now until we decide if/how we plan to open source and maintain/support this after March 14.
cc @jamiboym
There was a problem hiding this comment.
These two don't really matter until we actually open source it - I guess it would be better to remove it for now so its not in our commit history. (But I think we can chose how far back our commit history we can make public anyways?)
|
|
||
| const spawnAgent = (args) => { | ||
| const dirname = path.resolve(__dirname); | ||
| const filename = path.join(dirname, '../agent/main.py'); |
There was a problem hiding this comment.
Since path.join is already being used, I think we should use
const filename = path.join(dirname, '..', 'agent', 'main.py');to keep it platform agnostic.
|
This is now additionally out of date. Changes similar to those in #97 will need to be added to support the hole punching updates. |
|
Excited to see this piece finished! Thanks for the hard work! |
Created an atom plugin! Woo! Follows the protocol defined in the Wiki.
Added a similar dev setup script with symlinks. You will however need to install the required node modules before this will work.
Granted, this isn't launch blocking, so don't feel rushed to review.
Will add screenshots/screencasts here as well.
#30