Skip to content

Conversation

Logise1123
Copy link

No description provided.

@github-actions github-actions bot added the pr: new extension Pull requests that add a new extension label Apr 21, 2025
Copy link
Contributor

@Brackets-Coder Brackets-Coder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for consistent formatting why can't you open this structure up like the other blocks?

getInfo() {
return {
id: "FirebaseDB",
name: "Firebase DB",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should also translate the block text

};
}

async delay() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Scratch.fetch() instead of fetch()

Comment on lines 99 to 116
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"]
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally speaking you should use Scratch.Cast() on all of your arguments to avoid weird quirks... this goes for all blocks too

@Brackets-Coder
Copy link
Contributor

!format

1 similar comment
@CubesterYT
Copy link
Member

!format

class FirebaseDB {
constructor() {
this.api =
"https://guessthepin-2fe64-default-rtdb.europe-west1.firebasedatabase.app";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this references an external domain, it likely won't be merged. Here's a few reasons:

  1. If the domain ever goes down, real projects don't work any more -- there's no guarantee of future compatibility
  2. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: new extension Pull requests that add a new extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants