File loading #17

Azeirah · 2015-07-27T20:28:19Z

This pull request should be merged after the "fixes" one.

In this pull request, I've added the ability to load files by passing a filename to the node server.js filename.js command. This textfile will then be read, and sent to all open editors, that's not intended behavior, but that's how it works right now.

What I've done is, I've made a very light abstraction over websockets, to make it easy to work with. The server-side one is server/channel.js and the client-side one is lib/channel.js.

server/FileLoader.js does the actual work on the server side. Whenever a new websocket connection gets established between a Moonchild instance and the server, if the server received a file via the commandline, then it opens that file and sends a "fileLoad" event using the channel to all clients.

The client then handles this "fileLoad" event in editor/loadFile.js. On "fileLoad", it simply sets the editor content to the received content.

pdubroy · 2015-07-28T18:19:15Z

I'll take a look at this tomorrow. If you have a chance, would you mind re-syncing with the latest from cdglabs:master and uploading a new PR that only includes the file loading changes? It's a bit hard to review this way, and I'm not quite sure what the history will look like if I merge it like this...but I'm not an expert on GitHub PRs.

Azeirah · 2015-07-28T18:38:45Z

Yeah I expected that Github would show this PR without the first three commits after the first merge. Apparently it doesn't do that. It's why I didn't make a PR for the file saving yet either.

…leLoading Merge older changes into fileLoading

Azeirah · 2015-07-28T19:06:15Z

Oh alright, I've merged cdglabs/master onto fileLoading, and github automatically edited this PR.

pdubroy · 2015-08-03T09:44:07Z

editor/loadFile.js

@@ -0,0 +1,26 @@
+function createFileLoader (moonchild, channel) {
+  var fileLoader = Object.create(null);


This variable is unused.

pdubroy · 2015-08-04T11:30:15Z

common/channel.js

+    // the server requires a different websocket constructor, and it talks to multiple clients instead of one.
+    if (typeof config === "object") {
+        this.websocket = new webSocket.Server({server: config});
+        this.type = "server";


I would be tempted to pass the type in as an argument -- that way it's clear at the call sites what kind of channel you are instantiating. It's a little confusing that the type of channel is determined not by the value of any of the arguments, but by the type of the argument. I think passing in the type as a string would make it a bit less confusing.

Another approach would be to create a ClientChannel and a ServerChannel constructor, and export only those constructors -- they could call the Channel constructor with the appropriate args. I'm fine with whatever you prefer though.

Yeah, I think the way it currently is feels a little awkward, I don't like different constructors, because It's supposed to be the same api on the server and on the client.

Maybe something like this?

// client var channel = new Channel({ type: Channel.server, httpServer: server }); // client var channel = new Channel({ type: Channel.client, port: 8080 });

I like it. I would just do { type: 'server', ... }, but it's your call.

pdubroy · 2015-08-04T11:36:38Z

lib/util.js

+
+window.getParameterByName = getParameterByName;
+
+function formatString(string, arguments) {


It looks like this is no longer used.

pdubroy · 2015-08-04T11:39:47Z

This is looking good! I'm happy to merge it as soon as you fix up the lint errors.

Sorry it took a lot of back and forth, but I think the code is looking really good now.

Azeirah · 2015-08-04T12:28:30Z

Sorry it took a lot of back and forth

Oh that's no problem, code ended up a lot better than my initial request, it's really easy to overlook tiny mistakes.

Azeirah · 2015-08-04T15:05:41Z

Do you get notifications when I push to this branch? I've fixed the linter errors, had to add an exception for a warning in the uriParser regex, as well as add window to the global variables for the linter.

pdubroy · 2015-08-05T07:37:38Z

.eslintrc

@@ -9,6 +9,7 @@
    "console": true,
    "module": true,
    "require": true,
+    "window": true


Note the comment above: it says that if additional globals are required, they should be added per-file -- see http://eslint.org/docs/user-guide/configuring.html#specifying-globals.

pdubroy · 2015-08-05T07:46:26Z

I don't seem to get notifications when you push to this branch.

Apart from the comments I just left, one more thing: the new files you've added are not being checked by the linter. Can you edit the 'lint' script in package.json to make sure that they are getting checked? And also fix the errors.

I also noticed that the code under editor/ is also not being checked, but I can fix that later.

Azeirah · 2015-08-05T11:29:33Z

Yeah I noticed it was only linting the lib directory. Do you prefer 2-space indentation in the whole project? It's not consistent throughout the files. I strongly prefer 4-space indentation, I think it's far more readable.

pdubroy · 2015-08-05T11:34:28Z

Yes, I prefer 2-space indentation -- but third-party code should remain unchanged.

Which files are not using 2-space indentation now? I took a quick look and it seems like almost everything is.

Azeirah · 2015-08-05T11:36:12Z

Oh now that you say it, it's probably because of my personal preference to do 4-space indent that I thought it was inconsistent, nevermind that

Azeirah · 2015-08-05T11:44:34Z

Are console statements fine? ESlint is complaining about them, they're used throughout the project however. Should I add a global exception, or a per-file exception?

pdubroy · 2015-08-05T11:46:56Z

Yes, that's fine given the nature of the project. Might as well add a global exception.

Azeirah · 2015-08-05T11:52:41Z

Alright, no more linter errors, removed the common folder, and parseUri is the unmodified version. I think that's it!

pdubroy · 2015-08-05T12:43:38Z

It all looks good, but one more problem -- the tests are failing. I think the simplest way to fix it would just be to move the client Channel into the editor. Then we can think a bit more about what the architecture should be.

Or we just get rid of that test for now. It's not extremely useful.

Azeirah · 2015-08-05T13:23:29Z

I could also add a check for window to getParameterByName

function getParameterByName(name) {
  return parseUri(window.location.search || "").queryKey[name];
}

edit: Oh nevermind, that doesn't work.. It complains about any reference to window at all.
Heck, even after fixing the error, I get a ECONNREFUSED error, because websockets obviously don't work in the test environment.

The problem is that the test environment is not a browser, I'm not sure what you want to do with this.

pdubroy · 2015-08-05T13:32:32Z

It's a bit of a hack, but sure, something like that is fine for now.

Azeirah · 2015-08-05T13:36:20Z

It doesn't work though, because I get a websocket error after fixing the error. So I'd have to come up with another "fix" for that also, and for anything else. The real problem here is that the test environment is not a browser. Either the tests need to test only things that aren't browser dependent, or the test environment should be replaced.

And otherwise, there are temporary solutions like moving the script or removing the test, but that doesn't really solve the issue.

pdubroy · 2015-08-05T13:47:31Z

The real problem here is that the test environment is not a browser. Either the tests need to test only things that aren't browser dependent, or the test environment should be replaced.

Well, the test is not testing functionality that is browser dependent. I think the issue is that the architecture hasn't really been thought through yet. For now I think the best thing to do is to move the channel stuff out of moonchild.js and into editor.js -- that way we can land this PR without a whole lot more work. After that, we can think a bit more about what the division of responsibilities between the various modules should be, and address that separately. What do you think?

Azeirah · 2015-08-05T13:48:56Z

Alright that seems fine. It's not like this pr should be concerned with tests or architecture, it's just for file loading.

Azeirah · 2015-08-05T14:05:54Z

I can't use require in editor.js though, so I'd need to include the npm module websocket lib in index.html, and modify util.js to not use module.exports, oh well :|

Azeirah · 2015-08-05T14:22:02Z

Ok nevermind all this, it requires an avalanche of small changes throughout the project. I think it's best if the test gets removed for now, then think about the architecture, and then think about tests again.

Azeirah added 3 commits July 27, 2015 21:20

added the ability to load files

2a8c95d

Made server-side file loading a lot more modular

8531bef

removed unused function

730d3c2

Merge branch 'master' of https://github.com/cdglabs/moonchild into fi…

afc07d2

…leLoading Merge older changes into fileLoading

pdubroy reviewed Aug 3, 2015
View reviewed changes

Made changes as recommended by Patrick

705c060

pdubroy reviewed Aug 4, 2015
View reviewed changes

Fixed linter issues and made additional style changes

e8fb55d

pdubroy reviewed Aug 5, 2015
View reviewed changes

Resolved remaining linter errors

60a6950

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File loading #17

File loading #17

Azeirah commented Jul 27, 2015

pdubroy commented Jul 28, 2015

Azeirah commented Jul 28, 2015

Azeirah commented Jul 28, 2015

pdubroy Aug 3, 2015

pdubroy Aug 4, 2015

Azeirah Aug 4, 2015

pdubroy Aug 4, 2015

pdubroy Aug 4, 2015

pdubroy commented Aug 4, 2015

Azeirah commented Aug 4, 2015

Azeirah commented Aug 4, 2015

pdubroy Aug 5, 2015

pdubroy commented Aug 5, 2015

Azeirah commented Aug 5, 2015

pdubroy commented Aug 5, 2015

Azeirah commented Aug 5, 2015

Azeirah commented Aug 5, 2015

pdubroy commented Aug 5, 2015

Azeirah commented Aug 5, 2015

pdubroy commented Aug 5, 2015

Azeirah commented Aug 5, 2015

pdubroy commented Aug 5, 2015

Azeirah commented Aug 5, 2015

pdubroy commented Aug 5, 2015

Azeirah commented Aug 5, 2015

Azeirah commented Aug 5, 2015

Azeirah commented Aug 5, 2015

		@@ -0,0 +1,26 @@
		function createFileLoader (moonchild, channel) {
		var fileLoader = Object.create(null);


		window.getParameterByName = getParameterByName;

		function formatString(string, arguments) {

File loading #17

Are you sure you want to change the base?

File loading #17

Conversation

Azeirah commented Jul 27, 2015

pdubroy commented Jul 28, 2015

Azeirah commented Jul 28, 2015

Azeirah commented Jul 28, 2015

pdubroy Aug 3, 2015

Choose a reason for hiding this comment

pdubroy Aug 4, 2015

Choose a reason for hiding this comment

Azeirah Aug 4, 2015

Choose a reason for hiding this comment

pdubroy Aug 4, 2015

Choose a reason for hiding this comment

pdubroy Aug 4, 2015

Choose a reason for hiding this comment

pdubroy commented Aug 4, 2015

Azeirah commented Aug 4, 2015

Azeirah commented Aug 4, 2015

pdubroy Aug 5, 2015

Choose a reason for hiding this comment

pdubroy commented Aug 5, 2015

Azeirah commented Aug 5, 2015

pdubroy commented Aug 5, 2015

Azeirah commented Aug 5, 2015

Azeirah commented Aug 5, 2015

pdubroy commented Aug 5, 2015

Azeirah commented Aug 5, 2015

pdubroy commented Aug 5, 2015

Azeirah commented Aug 5, 2015

pdubroy commented Aug 5, 2015

Azeirah commented Aug 5, 2015

pdubroy commented Aug 5, 2015

Azeirah commented Aug 5, 2015

Azeirah commented Aug 5, 2015

Azeirah commented Aug 5, 2015