add server storage extension #451
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
image text dont fit let me fix rq |
Steve0Greatness
left a comment
Steve0Greatness
left a comment
There was a problem hiding this comment.
Sorry for the somewhat long review, but I had a couple thoughts.
| this.maxDataSize = 262144; | ||
|
|
||
| this.editMode = 'live'; // 'live' or 'local' | ||
| this.localCache = {}; // { key: value } |
There was a problem hiding this comment.
Cache is usually supposed to be updated quite a lot, so a regular object is probably the least efficient way to handle this. I'd personally recommend using a Map for caching, instead.
| this.apiKey = null; | ||
| this.maxDataSize = 262144; | ||
|
|
||
| this.editMode = 'live'; // 'live' or 'local' |
There was a problem hiding this comment.
This should be documented in JSDoc so that IDEs know what values it should be. In this case the specific comment you'd want here is /** @type {'live' | 'local'} */. Note that this doesn't have a runtime impact, it's only for development convenience and code documentation.
|
|
||
| class ServerStorage { | ||
| constructor() { | ||
| this.serverUrl = 'https://ikelene.dev/storage/'; |
There was a problem hiding this comment.
serverUrl, editMode and apiKey might be wanted to be saved in extension data using the serialization/deserialization APIs, as they're global state.
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Accept': 'application/json' | ||
| }, | ||
| body: JSON.stringify({ apiKey: this.apiKey }) |
There was a problem hiding this comment.
This would require changes to the server, but in general API key sending should be done through headers, such as Authorization using the basic method. If this is done, then the request could instead be sent as using the GET method, which better explains what the actual request is doing.
There was a problem hiding this comment.
This also applies to other POST requests that simply send the API key and are used to simply query data.
There was a problem hiding this comment.
API key should also be sent in headers of other data posting queries.
| const result = await res.json(); | ||
| if (!result.success || !Array.isArray(result.keys)) return '[]'; | ||
| return JSON.stringify(result.keys); | ||
| } catch (e) { |
There was a problem hiding this comment.
In general, if you're going to ignore values like this, you should set it's name to _. It makes it easier to tell that it's being ignored.
| console.warn('need api key to download cache'); | ||
| return; | ||
| } | ||
| try { |
There was a problem hiding this comment.
In general, if the function need to be able to perform networking and is unable to, it should be able to error out. Here it makes sense to dump the responsibility to handle it onto the user of the extension.
| if (response.status === 401) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I'm aware that I've said this a lot, but really don't be afraid to throw errors, especially when there is genuinely something wrong.
| constructor() { | ||
| this.serverUrl = 'https://ikelene.dev/storage/'; | ||
| this.apiKey = null; | ||
| this.maxDataSize = 262144; |
There was a problem hiding this comment.
Why specifically 2^18 bytes?
|
|
||
| const result = await response.json(); | ||
| if (result.success && result.data) { | ||
| return result.data.value || ''; |
There was a problem hiding this comment.
This API is weirdly complicated. You may want to trim back what it sends in the main body a little bit.
|
|
||
| try { | ||
| const response = await fetch(this.serverUrl + 'delete.php', { | ||
| method: 'POST', |
There was a problem hiding this comment.
There's actually a specific DELETE method made for the exact purpose. It's even allowed to have a request body.
|
Btw, just here to say that If you didn't add protection on the php script for setting data, ADD IT, people can just go there and prob find a way to create a php file and run it, and as you know, it gives access to the server, just sayin btw, gn |
3 participants
idk why it still wants to merge update url and comments (it was already merged)
and i did sync
anyways my new super cool extension is here
server is open source at https://github.com/Ikelene/scratchStorageExtension
thanks