Create firebasedb.js#2087
Conversation
github-actions
Bot
added
the
pr: new extension
Brackets-Coder
left a comment
Brackets-Coder
left a comment
There was a problem hiding this comment.
This is just a quick review going over TW extension best practices, I don't have time to do a full in-depth functionality review. To wrap up: use Scratch.translate and Scratch.Cast where necessary.
| @@ -0,0 +1,187 @@ | |||
| // Name: FirebaseDB | |||
| // ID: firebasedb | |||
There was a problem hiding this comment.
ID should include your username in both header metadata and the getInfo ID
| color1: "#fea631", | ||
| menuIconURI: icon, | ||
| blocks: [ | ||
| {blockType: Scratch.BlockType.LABEL, text: "Made by @logise on Discord"}, |
There was a problem hiding this comment.
Just for consistent formatting why can't you open this structure up like the other blocks?
| getInfo() { | ||
| return { | ||
| id: "FirebaseDB", | ||
| name: "Firebase DB", |
There was a problem hiding this comment.
To fix all of the lint errors you can just use Scratch.translate() on the extension and block names.
| { | ||
| opcode: "setKey", | ||
| blockType: Scratch.BlockType.COMMAND, | ||
| text: "set key [KEY] to value [VALUE]", |
There was a problem hiding this comment.
you should also translate the block text
| }; | ||
| } | ||
|
|
||
| async delay() { |
There was a problem hiding this comment.
Lint is returning Async method 'delay' has no 'await' expression. Avoid putting async before the extension's block functions, rather just put an asynchronous function inside them
| await this.delay(); | ||
| const { KEY, VALUE } = args; | ||
| if (VALUE.length > 8000) return; | ||
| await fetch(`${this.api}/pin/${encodeURIComponent(KEY)}.json`, { |
There was a problem hiding this comment.
Use Scratch.fetch() instead of fetch()
| async deriveKey(password, salt) { | ||
| const enc = new TextEncoder(); | ||
| const keyMaterial = await crypto.subtle.importKey( | ||
| "raw", enc.encode(password), "PBKDF2", false, ["deriveKey"] | ||
| ); | ||
| return await crypto.subtle.deriveKey( | ||
| { | ||
| name: "PBKDF2", | ||
| salt, | ||
| iterations: 100000, | ||
| hash: "SHA-256" | ||
| }, | ||
| keyMaterial, | ||
| { name: "AES-GCM", length: 256 }, | ||
| false, | ||
| ["encrypt", "decrypt"] | ||
| ); | ||
| } |
There was a problem hiding this comment.
It seems like all of the block functions are async for interacting with the database server. Is it necessary to do it like this or can you put an asynchronous function inside all of them? Also, you might want to include documentation noting this because I don't remember if the script pauses until each async block finishes executing or continues while the asyncs are still executing
|
|
||
| async getKeyWithPassword(args) { | ||
| await this.delay(); | ||
| const { KEY, PASSWORD } = args; |
There was a problem hiding this comment.
Generally speaking you should use Scratch.Cast() on all of your arguments to avoid weird quirks... this goes for all blocks too
|
!format |
1 similar comment
|
!format |
| class FirebaseDB { | ||
| constructor() { | ||
| this.api = | ||
| "https://guessthepin-2fe64-default-rtdb.europe-west1.firebasedatabase.app"; |
There was a problem hiding this comment.
If this references an external domain, it likely won't be merged. Here's a few reasons:
- If the domain ever goes down, real projects don't work any more -- there's no guarantee of future compatibility
- Some users may be on a whitelisted network that blocks this anyway.
I'd recommend closing this PR and working with the already open #1341, which uses a browser API for a more compatible permanent storage solution.
|
As nice of an extension this is, Im closing it for several reasons:
|
5 participants
No description provided.