Skip to content

Use EventTarget rather than the Node "events" module#1188

Merged
microbit-matt-hillsdon merged 6 commits into
mainfrom
typed-event-target
Jul 9, 2024
Merged

Use EventTarget rather than the Node "events" module#1188
microbit-matt-hillsdon merged 6 commits into
mainfrom
typed-event-target

Conversation

@microbit-matt-hillsdon

@microbit-matt-hillsdon microbit-matt-hillsdon commented Jul 8, 2024

Copy link
Copy Markdown
Collaborator

This should amount no meaningful change but is a useful preliminary to extracting the device code for reuse.

Incorporates the only meaningful file from https://github.com/DerZade/typescript-event-target/ (with licence details) to save on the dependency.

Compiles but untried
Forces us to remove the clean up in dispose so perhaps some kind of
assertion should replace it.
@github-actions

github-actions Bot commented Jul 8, 2024

Copy link
Copy Markdown

Comment thread src/device/webusb.ts Outdated
@microbit-matt-hillsdon microbit-matt-hillsdon changed the title [DO NOT MERGE] Try another approach to typing event target Use EventTarget rather than the Node "events" module Jul 8, 2024
Comment thread src/e2e/simulator.test.ts

const buttonTest =
"from microbit import *\nwhile True:\nif button_a.was_pressed():\ndisplay.show(Image.NO)";
const buttonTest = `from microbit import *

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I've corrected the indentation in these tests which surprised me when I saw them in failure screenshots (for reasons unrelated to the indentation).

},
[setSelection]
);
useEffect(() => {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think this was a listener ordering thing. I've changed it to avoid making a state update at all.

Comment thread src/device/mock.ts
async initialize(): Promise<void> {}

dispose() {
this.removeAllListeners();

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We can't do this anymore as EventTarget doesn't have such API.

All listener add/removes are correctly paired except that in host.ts and a test, neither of which need to remove their listeners.

@microbit-matt-hillsdon microbit-matt-hillsdon marked this pull request as ready for review July 8, 2024 14:14
Comment thread package.json

@microbit-robert microbit-robert left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM.

@microbit-matt-hillsdon microbit-matt-hillsdon merged commit 506446c into main Jul 9, 2024
@microbit-matt-hillsdon microbit-matt-hillsdon deleted the typed-event-target branch July 9, 2024 16:38
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