edemaine / cocreate

Cocreate Shared Whiteboard/Drawing
MIT License
209 stars 27 forks source link

Add API Endpoint for Creating New Rooms #51

Closed adqm closed 4 years ago

adqm commented 4 years ago

This is a first attempt at adding an API endpoint for creating new rooms (#49), as well as a more general structure for future API methods, under the assumption that more API endpoints might want to show up later.

In its current form, each endpoint is defined by a function that optionally takes a query string, as well as all of the raw pieces that WebApp provides. Raw access to those objects means that the endpoint can construct its own response if it wants to, or it can return a [statuscode, object] pair, from which an appropriate response will be constructed. This also assumes that each API endpoint is responsible for its own error handling

Then the roomNew endpoint is implemented using this structure, which creates a new room and returns its ID.

I'm not sure if this structure actually makes sense, or if there's a better way to do it. Feedback welcome!

adqm commented 4 years ago
* Fixed a bug: `url` was used for a package name and a parsed URL

Ah, are coffeescript's scoping rules such that that would actually overwrite the global binding, rather than shadowing it?

* Fixed a sort-of bug: it's `.search` not `.query`

I don't think that's true? When I console.log-ed things, I see that url.query is an object that contains the parsed querystring parameters. Is there something I'm missing?

If it works as-is, that's totally cool, too; just curious.

* Added `grid` as an argument instead of forcing `true`

Ah, good. I like that.

* Reformatted to match my CoffeeScript style

Sounds good.

I also like the change to have the functions return objects instead of arrays.

edemaine commented 4 years ago

Ah, are coffeescript's scoping rules such that that would actually overwrite the global binding, rather than shadowing it?

Yep! It's an annoying source of bugs in CoffeeScript, but is also handy... As mentioned here, every variable in CoffeeScript is as if it was declared nonlocal in Python.

* Fixed a sort-of bug: it's `.search` not `.query`

I don't think that's true? When I console.log-ed things, I see that url.query is an object that contains the parsed querystring parameters. Is there something I'm missing?

I was going based on this documentation... but I didn't scroll down all the way to this. So you seem to be right: .query is an object because you passed in true to url.parse.

But I also see that url.parse is deprecated, and you're supposed to use new URL now, which doesn't have this argument. Then I think you need to use searchParams. (But as I said, I haven't tested.)

I also like the change to have the functions return objects instead of arrays.

Oh yeah, forgot to mention that. It's more JavaScripty. 🙂

edemaine commented 4 years ago

OK, I got a chance to test, and made various fixes in 3cafb0e06c0ed478b5d9c05e11f7bfaa532669de, including switch to new URL, proper use of .searchParams (need to use .get), JSON-parse room argument so false and 0 work, and change path to pathname. I think this is good to go now.

adqm commented 4 years ago

Looks good to me, thanks for taking a look! Just the one remaining question about the Content-type header in the 404 case.