8000 Added connection-specific interfaces and factory functions for exporting instead by microbit-grace · Pull Request #50 · microbit-foundation/microbit-connection · GitHub
[go: up one dir, main page]
Skip to content

Added connection-specific interfaces and factory functions for exporting instead #50

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Feb 17, 2025
Merged

Added connection-specific interfaces and factory functions for exporting instead #50

merged 13 commits into from
Feb 17, 2025

Conversation

microbit-grace
Copy link
Contributor
@microbit-grace microbit-grace commented Jan 23, 2025 •
edited
Loading
  • Added specific interfaces for different connection types. This is helpful for mocking.
  • Exported new functions for creating the connections instead of the class.
  • Updated demo and docs to match changes.

@microbit-grace microbit-grace mentioned this pull request Jan 23, 2025
Merged
@cloudflare-workers-and-pages Cloudflare Workers and Pages
Copy link
cloudflare-workers-and-pages bot commented Jan 23, 2025 •
edited
Loading

Deploying microbit-connection with  Cloudflare Pages  Cloudflare Pages

Latest commit: 65e8de4
Status: ✅  Deploy successful!
Preview URL: https://0f4c4aa4.microbit-connection.pages.dev
Branch Preview URL: https://mock-connections.microbit-connection.pages.dev

View logs

@microbit-grace microbit-grace changed the title [WIP - DO NOT MERGE] Interface for mocking connections Interface for mocking connections Jan 24, 2025
@microbit-grace microbit-grace marked this pull request as ready for review January 24, 2025 13:54
microbit-grace
microbit-grace commented Jan 24, 2025
View reviewed changes
@microbit-matt-hillsdon
Copy link
Contributor

Let's:

  • Rename the implementations to e.g. MicrobitWebUSBConnectionImpl and the interfaces to e.g. MicrobitWebUSBConnection
  • Move the interfaces to the relevant files (e.g. usb.ts).
  • Stop exporting the implementation classes by introducing createWebUSBConnection(options) function and similar and exporting that instead

Then client code will need to be updated to call the function instead of the constructor but otherwise stay the same in terms of type names.

Please can we also look at whether there's a better approach to typing the events? Ideally we'd be able to add common events only via DeviceConnection and events that only applied e.g. USB would only be available via that interface.

microbit-matt-hillsdon
microbit-matt-hillsdon reviewed Feb 14, 2025
View reviewed changes
src/demo.ts Outdated
@@ -194,7 +195,7 @@ const createConnectSection = (type: ConnectionType): Section => {
};

const createFlashSection = (): Section => {
if (!connection.flash) {
if (typeof connection["flash"] !== "function") {
8000
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use ConnectionType to determine this.

Copy link
Contributor Author
@microbit-grace microbit-grace Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Have addressed this here: 29257d6

@microbit-grace
Copy link
Contributor Author

See below for commits addressing the comments:

  • Rename the implementations to e.g. MicrobitWebUSBConnectionImpl and the interfaces to e.g. MicrobitWebUSBConnection

96b4af3, 9ab8849

  • Move the interfaces to the relevant files (e.g. usb.ts).

1d47926

  • Stop exporting the implementation classes by introducing createWebUSBConnection(options) function and similar and exporting that instead

c2ab488, 29257d6

Please can we also look at whether there's a better approach to typing the events? Ideally we'd be able to add common events only via DeviceConnection and events that only applied e.g. USB would only be available via that interface.

Maybe like this e5929d2?

Have also updated the documentation here: 65e8de4

@microbit-grace microbit-grace changed the title Interface for mocking connections Added connection-specific interfaces and factory functions for exporting instead Feb 14, 2025
@microbit-matt-hillsdon
Copy link
Contributor

Code changes look good. I'd like to spend a bit of time playing around with how it feels to use before we go ahead.

microbit-robert
microbit-robert approved these changes Feb 17, 2025
View reviewed changes
Copy link
Contributor
@microbit-robert microbit-robert left a comment

Choose a reason for hiding this comment

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

I think this approach makes sense. Looks good.

@microbit-grace microbit-grace merged commit dc51d00 into main Feb 17, 2025
4 checks passed
@microbit-grace microbit-grace deleted the mock-connections branch February 17, 2025 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@microbit-matt-hillsdon microbit-matt-hillsdon microbit-matt-hillsdon approved these changes

@microbit-robert microbit-robert microbit-robert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@microbit-grace @microbit-matt-hillsdon @microbit-robert
0