[WIP] Provide an easy way for user to import a project.. #150

Ilphrin · 2016-09-27T10:07:04Z

...to fix #46

elisee

Hi and thanks a lot for the PR!

This is looking good overall! made a bunch of initial comments, please address them or reply and then I'll test and look into the manifest / actualIndex thingy you mentioned in #46.

elisee · 2016-10-02T04:51:00Z

client/src/hub/index.jade

 button(disabled).open-project= t("hub:openProject")
 button(disabled).edit-project= t("hub:editDetails.title")

+ form(style="display: none")
+ input.file-select(type="file", accept="application.zip")


I think this should be application/zip instead?

Also maybe we can have a more specific class name like project-archive-select

elisee · 2016-10-02T04:51:16Z

client/src/hub/index.jade

 button(disabled).open-project= t("hub:openProject")
 button(disabled).edit-project= t("hub:editDetails.title")

+ form(style="display: none")


Best to use hidden rather than CSS

elisee · 2016-10-02T04:52:15Z

client/src/hub/index.ts

+ let target = event.target as HTMLInputElement;
+ if (target.files.length === 0) return;
+
+ let reader = new FileReader();


target and reader can be made const too.

elisee · 2016-10-02T04:52:36Z

client/src/hub/index.ts

+ let reader = new FileReader();
+
+ reader.onload = function(evnt) {
+ socket.emit("import:projects", {name: target.files[0].name.split(".")[0], data: (evnt.target as any).result});


Make sure to have spaces inside curly brackets

elisee · 2016-10-02T04:53:13Z

package.json

@@ -56,6 +56,7 @@
 "socket.io": "^1.4.4",
 "tab-strip": "^2.3.0",
 "tv4": "^1.2.7",
+ "unzip": "^0.1.11",


We already have yauzl (which stands for "yet another unzip library"), would be better to reuse it rather than add a dependency on another ZIP lib.

elisee · 2016-10-02T04:53:35Z

server/RemoteHubClient.ts

@@ -190,4 +199,29 @@ export default class RemoteHubClient extends BaseRemoteClient {
 else callback(null);
 });
 };
+
+ private onImportProject = (data: ImportProjectData) => {


There's an extra space after the =

elisee · 2016-10-02T04:55:03Z

server/RemoteHubClient.ts

+
+ private onImportProject = (data: ImportProjectData) => {
+
+ fs.writeFile(path.join(this.server.projectsPath, "project.zip"), data.data as string, (err) => {


A bit dangerous to write the ZIP file directly inside the project folder, since it could be overwritten by another import at the same time. If saving it to disk is actually needed (can't we unzip from memory?), would be best to save it in the system's temporary folder with a random name. We might already do that elsewhere, look for yauzl usage I guess.

elisee · 2016-10-02T04:59:05Z

server/RemoteHubClient.ts

+ })
+ .on("close", () => {
+ this.server.loadProject(data.name, (err: Error) => {
+ if (err) {


We use explicit if (err != null) throughout the codebase rather than implicit boolean coercion.

elisee · 2016-10-02T05:05:05Z

server/RemoteHubClient.ts

+ if (err) {
+ console.log(err);
+ }
+ });


Regarding #46 (comment):

I think you'll need to change loadProject in ProjectHub.ts to pass you the newly initialized project server as a second argument, then you can access the manifest (which contains name, description, etc. to send it to clients for display in the hub).

For actualIndex, I think after loading the project, we'll need to add the ProjectHub's data.projects so that it shows up in the list, and that will give you that index. If that's too confusing I can take care of it.

Ilphrin · 2016-10-04T14:02:10Z

Hi!
Thanks for your review! I'm am changing my modifications, and use yauzl instead of unzip. I need to other dependencies to make it work: node-temp for temporary folder management, and copy-dir to move the content of the extracted folder, may I add them as dependencies with npm? :)

unzip -> yauzl Use node-temp to create a temporary directory Use node-mv to move the directory

Ilphrin · 2016-10-06T12:18:30Z

@elisee Can you explain what you mean by adding data.projects?

- Based on review from elisee - Use copy-dir, temp and yauzl

elisee

Looking pretty good, listed a few more things to fix and hopefully made it clearer what's required re. the manifest.

elisee · 2016-10-29T04:03:16Z

server/RemoteHubClient.ts

@@ -14,6 +21,10 @@ interface ProjectDetails {
 icon: Buffer;
 }
 interface AddProjectCallback { (err: string, projectId?: string): any; };
+interface ImportProjectData {
+ name: string;
+ data: string;


I think this should be a Buffer not a string? Since it's an ArrayBuffer on the client-side, should be converted to a NodeJS Bon the server-side.

Oups, it was an inattention mistake. I change this!

elisee · 2016-10-29T04:04:17Z

server/RemoteHubClient.ts

+ });
+ zipFile.on("end", () => {
+ // move the extracted content into the projects' folder
+ copydir(path.join(dirPath, rootFolderName), path.join(this.server.projectsPath, rootFolderName), (err: Error) => {


Is there any reason why we want to copy rather than just doing fs.rename on the root dir? Seems better to avoid having a copy lying around, even if it's in a temp folder.

Well, when I tried with fs.rename I had a crash on the server side with on error like this:

Error: EXDEV: cross-device link not permitted, rename '/tmp/project1161019-14225-1cpphb6/dodgell-master/' -> '/home/ilphrin/Jeux/superpowers/core/projects/dodgell-master/'

I didn't find a solution to this, so I tried copying the folder and then it worked

elisee · 2016-10-29T04:07:00Z

server/RemoteHubClient.ts

+ copydir(path.join(dirPath, rootFolderName), path.join(this.server.projectsPath, rootFolderName), (err: Error) => {
+ if (err != null) throw err;
+ this.server.loadProject(rootFolderName, (err: Error, manifest: SupCore.Data.ProjectManifest) => {
+ if (err != null) throw err;


Here we still need to:

Add the project manifest to this.server.projects (see here:

superpowers-core/server/RemoteHubClient.ts

Line 73 in 70f9b5b

this.server.data.projects.add(manifest, sortedIndex, (err: string, actualIndex: number) => {

). This will make it appear in the list when people reload the page.

And then we need to notify all people already connected to the hub of the new project (see

superpowers-core/server/RemoteHubClient.ts

Line 122 in 70f9b5b

this.server.io.in("sub:projects").emit("add:projects", manifest, actualIndex);

).

Otherwise the new project will be imported but you'll need to restart the server before it can be joined.

elisee · 2019-11-07T16:31:05Z

Closing since this has seen no activity in years.

Provide an easy way for user to import a project

89a1dc5

Ilphrin changed the title ~~[WIPProvide an easy way for user to import a project..~~ [WIP] Provide an easy way for user to import a project.. Sep 27, 2016

Ilphrin and others added 2 commits September 28, 2016 15:49

Add loadProject call

08980a4

Make importedFile const

a231a85

elisee suggested changes Oct 2, 2016

View reviewed changes

elisee reviewed Oct 2, 2016

View reviewed changes

Changed libraries

d85f94d

unzip -> yauzl Use node-temp to create a temporary directory Use node-mv to move the directory

Changed almost everything:

3dd9ec7

- Based on review from elisee - Use copy-dir, temp and yauzl

elisee suggested changes Oct 29, 2016

View reviewed changes

elisee force-pushed the master branch from 7e10189 to 00af9c5 Compare March 18, 2019 11:59

elisee closed this Nov 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Provide an easy way for user to import a project.. #150

[WIP] Provide an easy way for user to import a project.. #150

Ilphrin commented Sep 27, 2016 •

edited

elisee left a comment

elisee Oct 2, 2016

elisee Oct 2, 2016

elisee Oct 2, 2016

elisee Oct 2, 2016

elisee Oct 2, 2016

elisee Oct 2, 2016

elisee Oct 2, 2016

elisee Oct 2, 2016

elisee Oct 2, 2016

elisee Oct 2, 2016

Ilphrin commented Oct 4, 2016 •

edited

Ilphrin commented Oct 6, 2016

elisee left a comment

elisee Oct 29, 2016

Ilphrin Nov 19, 2016

elisee Oct 29, 2016

Ilphrin Nov 19, 2016

elisee Oct 29, 2016

elisee commented Nov 7, 2019


		private onImportProject = (data: ImportProjectData) => {

		fs.writeFile(path.join(this.server.projectsPath, "project.zip"), data.data as string, (err) => {

[WIP] Provide an easy way for user to import a project.. #150

[WIP] Provide an easy way for user to import a project.. #150

Conversation

Ilphrin commented Sep 27, 2016 • edited

elisee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ilphrin commented Oct 4, 2016 • edited

Ilphrin commented Oct 6, 2016

elisee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elisee commented Nov 7, 2019

Ilphrin commented Sep 27, 2016 •

edited

Ilphrin commented Oct 4, 2016 •

edited