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

Files #8

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Files #8

wants to merge 7 commits into from

Conversation

Azeirah
Copy link
Contributor

@Azeirah Azeirah commented May 9, 2015

There it is, file loading support for Moonchild.

Right now, you can open a file using
node server.js [file.js], any open instance of moonchild will then receive this file. Saving has been implemented, but it has been disabled to prevent potential issues, since it hasn't been tested at all, don't want to overwrite random files...

On the client side, channel.js, fileloader.js and util.js were added. These are fairly clean files. On the server side, I've bruteforced websockets into the code. Merging is not recommended before that is cleaned up or moved to a separate file.

I've also cleaned up some bits of the code, added some semicolons, refactored some ternary statements to if statements etc...

Also, I saw you were using, it's a bit cleaner to fix that up:

if (bla != null) {...}
// you can use
if (!bla) {...}

var a;
a === undefined; // true
a === null; // false
// by default, variables are "undefined", not "null"

Is it possible to request to merge this into a branch on cdglabs? Because this definitely shouldn't get merged into master right now

@pdubroy
Copy link
Collaborator

pdubroy commented May 10, 2015

Thanks for writing this! I will give it a try and a closer look tomorrow.

About the style: a couple of the files were originally written in CoffeeScript, which is why some things are different from typical JS style -- they are mostly just what the CS compiler produces. The reason it produces x != null is because !x will be true if x is 0 or '', whereas x != null checks only for null and undefined.

If this isn't ready to merge yet, we might as well keep it in your branch -- I'm not sure what the advantage would be of merging it into a branch in the main repo. I think the best thing to do is to get the smallest possible piece working -- say, file loading -- and get that merged into master.

@Azeirah
Copy link
Contributor Author

Azeirah commented May 14, 2015

Alright, I split the saving and the websockets part up into two files that get included by server.js. Loading and saving files is working on my system, I think it's ready to get merged.

@pdubroy
Copy link
Collaborator

pdubroy commented May 21, 2015

Hey Martijn, thanks a lot for this! I'm sorry that I haven't had a chance to give it a thorough review yet. We're trying to finish a release of another project we're working on. I hope I can take a look tomorrow, but I might not get to it until early next week.

@pdubroy
Copy link
Collaborator

pdubroy commented Jul 16, 2015

I'm really sorry I haven't gotten around to merging this. Would it be possible to split this up into a few different pull requests? That would make it easier for me to review. At the length it is now, it's a bit too much to handle at once.

In particular, it would be nice to have the style changes in a separate PR, and maybe loading and saving in separate PRs (if that makes sense).

@Azeirah
Copy link
Contributor Author

Azeirah commented Jul 16, 2015

Oh wow, looking at the changed files, that's a lot more than I remember changing, most changes are small insignificant style changes though, some done automatically by Sublime (removing trailing whitespace and adding enter at each file). I'll try to separate these changes from the actual fileloading.

As a quick synopsis, what I did was add a small abstraction over websockets that works like this:

// on the server
channel.send("someEvent", {value: "hi", stuff: 14, whatever: "yes please"});

// on the client
channel.on("someEvent", function (data) {
  // do whatever
});

These are implemented separately for the server and for the client. The client one is in /lib/channel.js, the server one is in server/channel.js.

Then, the actual fileloading is done by server/fileLoader.js and editor/loadFile.js (why did I not name them the same? I don't know!). I've written them in such a way that they can be loaded as a plugin.

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.

2 participants