Set up server using express #21

joachimvh · 2020-07-10T12:53:01Z

This allows setting up the (very basic) Solid server using node bin/server.js (with optionally -p port). Until there is dependency injection this class also handles the linking of all the classes.

I looked into Koa and the implementation would pretty much be 100% the same due to so little express code being used. Since there already was a dependency on express through another dependency I went for that one.

Some changes from the architecture:

The constructor takes a single handler instead of an array. Currently didn't see the point of an array if we have CompositeAsyncHandler.
The listen function returns an http.Server, makes testing much easier and makes it more similar to the actual listen interface of http.Server.

Currently CORS is handled in the new server class. An OptionOperationHandler might be more in line with the rest of the architecture, but currently there is not really a clean way for such a handler to add headers to the output. I do think we will need this at some point so potentially this can be changed then.

I looked into making the server extend http.Server to be consistent with that interface, but then many more overloads for listen are necessary, and making everything work with express is going to be much more of a hassle since there you only get a Server object when you call the listen function.

This PR also fixed the identifier extraction and a bug in the SimpleResourceStore.

RubenVerborgh · 2020-07-10T13:49:18Z

bin/server.ts

+import { SimpleRequestParser } from '../src/ldp/http/SimpleRequestParser';
+import { SimpleResourceStore } from '../src/storage/SimpleResourceStore';
+import { SimpleResponseWriter } from '../src/ldp/http/SimpleResponseWriter';
+import { SimpleTargetExtractor } from '../src/ldp/http/SimpleTargetExtractor';


We probably want a /src/index.ts that exports them all, and then import them from there.

export { SimpleResponseWriter } from '../src/ldp/http/SimpleResponseWriter'; etc.

👍 Will also need this for Components.js.

RubenVerborgh · 2020-07-10T13:50:32Z

bin/server.ts

+
+let port = 3000;
+
+if (process.argv.length >= 4 && process.argv[2] === '-p') {


Mmmm 🙂 shall we immediately go for something more robust?

https://www.npmjs.com/package/minimist

https://www.npmjs.com/package/yargs

https://www.npmjs.com/package/commander

Yea I didn't bother since this file should be very temporary, but it's true that such things have the tendency to then stay where they are

Just indicate it as such then?

RubenVerborgh · 2020-07-10T13:51:15Z

bin/server.ts

+const authorizer = new SimpleAuthorizer();
+
+// Will have to see how to best handle this
+const store = new SimpleResourceStore('http://localhost:3000/');


Probably want to inject port in there too?

RubenVerborgh · 2020-07-10T13:52:04Z

bin/server.ts

+httpServer.listen(port);
+
+// eslint-disable-next-line no-console
+console.log(`Running on port ${port}.`);


or process.stdout.print (don't forget to add newline)

And actually, log the full URL (so it's clickable etc)

RubenVerborgh · 2020-07-10T13:52:43Z

package.json

@@ -2,12 +2,13 @@
  "name": "@solid/community-server",
  "private": true,
  "version": "0.0.1",
-  "main": "index.js",
+  "main": "bin/server.js",


Oh no, this should the bin property. main is the code entry point.

That reminds me that we also need a typings entry, like here: https://github.com/rubensworks/jsonld-context-parser.js/blob/master/package.json#L14

Apparently the typings field is ignored if you also have a files field.

Also note that if your package.json includes the "files" property the "types" property will be ignored. In that case you need to pass your main declaration file to the "files" property as well.

src/storage/SimpleResourceStore.ts

test/integration/AuthenticatedLdpHandler.test.ts

test/unit/ldp/http/SimpleTargetExtractor.test.ts

RubenVerborgh · 2020-07-10T14:03:32Z

test/unit/server/ExpressHttpServer.test.ts

+
+class SimpleHttpHandler extends HttpHandler {
+  public async canHandle(): Promise<void> {
+    return undefined;


I've seen this a couple of times now, and always do a double-take.
Should we make this a Promise<boolean> (but still make it the contract that it's either true or an error)?
Or alternatively, rename this to ensureSupported or whatever?

I have no strong opinions on this so can go any way for me

Is just return possible actually? It's the undefined that makes me think.

The linter then complains about having an unnecessary return statement. And if you completely remove it, it complains about having an empty function. :D

I'd suggest boolean then; can be a separate PR.

For completeness, the previous discussion where this changed to void was here

test/unit/server/ExpressHttpServer.test.ts

Comments were also added and URI generation takes slash into account

rubensworks

Nothing major to add next to @RubenVerborgh's review.

rubensworks · 2020-07-13T12:44:24Z

bin/server.ts

+import { SimpleRequestParser } from '../src/ldp/http/SimpleRequestParser';
+import { SimpleResourceStore } from '../src/storage/SimpleResourceStore';
+import { SimpleResponseWriter } from '../src/ldp/http/SimpleResponseWriter';
+import { SimpleTargetExtractor } from '../src/ldp/http/SimpleTargetExtractor';


👍 Will also need this for Components.js.

rubensworks · 2020-07-13T12:49:09Z

package.json

@@ -2,12 +2,13 @@
  "name": "@solid/community-server",
  "private": true,
  "version": "0.0.1",
-  "main": "index.js",
+  "main": "bin/server.js",


That reminds me that we also need a typings entry, like here: https://github.com/rubensworks/jsonld-context-parser.js/blob/master/package.json#L14

joachimvh · 2020-07-14T09:35:38Z

Latest push should fix most issues (except handle function returning a boolean, that will be for later).

I used the cors package now, but the allow-origin header still wasn't correct out of the box. By default it would always return * so I still had to add a line for that here.

Add changes of resource mapper

joachimvh added 🐛 bug Something isn't working ☀️ enhancement New feature or request labels Jul 10, 2020

joachimvh requested a review from rubensworks July 10, 2020 12:53

RubenVerborgh approved these changes Jul 10, 2020

View reviewed changes

joachimvh mentioned this pull request Jul 13, 2020

Some bugfixes #24

Merged

joachimvh added 2 commits July 13, 2020 11:26

fix: Take host into account when extracting identifier

4928e4f

fix: Prevent data from being drained when read

5b3237f

Comments were also added and URI generation takes slash into account

rubensworks requested changes Jul 13, 2020

View reviewed changes

feat: Set up server using express

ed5b09a

joachimvh force-pushed the express branch from 42bc711 to ed5b09a Compare July 14, 2020 09:30

rubensworks approved these changes Jul 15, 2020

View reviewed changes

joachimvh merged commit a9dc59b into master Jul 15, 2020

joachimvh deleted the express branch July 15, 2020 08:59

joachimvh pushed a commit that referenced this pull request Sep 14, 2020

Merge pull request #21 from thdossch/merge

082400b

Add changes of resource mapper

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set up server using express #21

Set up server using express #21

joachimvh commented Jul 10, 2020

RubenVerborgh Jul 10, 2020

rubensworks Jul 13, 2020

RubenVerborgh Jul 10, 2020

joachimvh Jul 10, 2020

RubenVerborgh Jul 10, 2020

RubenVerborgh Jul 10, 2020

RubenVerborgh Jul 10, 2020

RubenVerborgh Jul 10, 2020

RubenVerborgh Jul 10, 2020

rubensworks Jul 13, 2020

joachimvh Jul 14, 2020

rubensworks Jul 14, 2020

RubenVerborgh Jul 10, 2020

joachimvh Jul 10, 2020

RubenVerborgh Jul 10, 2020

joachimvh Jul 10, 2020

RubenVerborgh Jul 10, 2020

joachimvh Jul 10, 2020

rubensworks left a comment

rubensworks Jul 13, 2020

rubensworks Jul 13, 2020

joachimvh commented Jul 14, 2020


		let port = 3000;

		if (process.argv.length >= 4 && process.argv[2] === '-p') {

Set up server using express #21

Set up server using express #21

Conversation

joachimvh commented Jul 10, 2020

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

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

rubensworks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joachimvh commented Jul 14, 2020