Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix promise rejection handling #207

Closed
wants to merge 1 commit into from
Closed

Conversation

FurryR
Copy link

@FurryR FurryR commented Apr 27, 2024

Resolves

N/A

Proposed Changes

Now waitPromise throws when promise rejected.

Reason for Changes

Already explained in Discord.

Test Coverage

Not ready yet.

@FurryR
Copy link
Author

FurryR commented Apr 27, 2024

Extension for test unit:

class Test {
  getInfo() {
    return {
      id: 'test',
      name: 'test',
      blocks: [
        { blockType: 'command', opcode: 'test', arguments: {}, text: 'test' }
      ]
    };
  }
  async test() {
    throw new Error('test');
  }
}
Scratch.extensions.register(new Test());

@GarboMuffin
Copy link
Member

GarboMuffin commented May 9, 2024

This does not match interpreter behavior at all

image

Scratch.extensions.register({
    getInfo: () => ({
        id: 'test123',
        name: 'test123',
        blocks: [
            {
                blockType: Scratch.BlockType.COMMAND,
                opcode: 'thr',
                text: 'return rejected promise'
            }
        ]
    }),
    thr: () => Promise.reject(new Error('Test error'))
});

Interpreter handles rejected promise by popStack()ing and continuing (inside a loop it works like JS continue;)

Current compiler handles rejected promise by just continuing.

This PR handles rejected promise by making the sequencer throw an uncaught error, breaking execution script of threads after for 1 tick, and then the thread is permanently stuck in an error'd state where it is highlighted but not actually running until the user restarts it. This is not better.

@GarboMuffin GarboMuffin closed this May 9, 2024
GarboMuffin added a commit that referenced this pull request May 9, 2024
Before this command:
- Interpreter would popStack() and continue. Inside a loop this was like JS continue, outside it was like return
- Compiler would return literal undefined and keep going.

After this commit:
Both return the error as a string and then continue as normal.
This has several benefits:
- Users can actually see the error messages
- Logical execution flow
- Consistent behavior
- Blocks aren't even supposed to return rejected promises anyways
- No vanilla blocks return rejected promises so no compatibility concerns

Supersedes #207
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants