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.

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

✨ (repo) [DSDK-558]: Extract transports into their own packages #474

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

✨ (repo) [DSDK-558]: Extract transports into their own packages #474

wants to merge 5 commits into from
+2,347 −1,365

Conversation

valpinkman
Copy link
Member

@valpinkman valpinkman commented Nov 5, 2024 •
edited
Loading

📝 Description

Extract transport to their own packages

A lot of rework here, you can have a breakdown of the steps here #454 (unfortunately with the renaming of the packages before we merged this it was too complicated to rebase)

  • Un-inversified the transports
  • Move some components back into the api folder of the dmk
  • extracted each transport into it's own package
  • correctly renamed the mockserver client and transport
  • added a TransportService to handle the transport inside the dmk
  • added and fixed some tests
  • updated the sample app to reflect the new api

❓ Context

  • Feature:

✅ Checklist

Pull Requests must pass CI checks and undergo code review. Set the PR as Draft if it is not yet ready for review.

  • Covered by automatic tests
  • Changeset is provided
  • Impact of the changes:
    • move needed packages to /api
    • universify transports
    • export transports into their own module

🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.

@vercel Vercel
Copy link

vercel bot commented Nov 5, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
device-sdk-ts-sample ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 15, 2024 9:26am
doc-device-management-kit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 15, 2024 9:26am

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024 •
edited
Loading

Messages
✅ Danger: All checks passed successfully! 🎉

Generated by 🚫 dangerJS against 0859764

@valpinkman valpinkman force-pushed the feature/dsdk-558-extract-transport branch from 769435c to 309ecb0 Compare November 5, 2024 16:04
@valpinkman valpinkman force-pushed the feature/dsdk-558-extract-transport branch from 309ecb0 to 65642da Compare November 5, 2024 16:15
@valpinkman valpinkman force-pushed the feature/dsdk-558-extract-transport branch from 65642da to c5dcb98 Compare November 5, 2024 16:53
@valpinkman valpinkman force-pushed the feature/dsdk-558-extract-transport branch from c5dcb98 to 158aebb Compare November 6, 2024 16:50
@valpinkman valpinkman marked this pull request as ready for review November 6, 2024 16:51
@valpinkman valpinkman requested a review from a team as a code owner November 6, 2024 16:51
@valpinkman valpinkman force-pushed the feature/dsdk-558-extract-transport branch from 158aebb to 18652b1 Compare November 6, 2024 16:57
ofreyssinet-ledger
ofreyssinet-ledger reviewed Nov 7, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
jdabbech-ledger
jdabbech-ledger requested changes Nov 7, 2024
View reviewed changes
Copy link
Contributor

@jdabbech-ledger jdabbech-ledger left a comment

Choose a reason for hiding this comment

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

Great refactor 👍
Just a few comments about import TransportFactory and few naming stuffs.
You could also create TransportService interface to implement it from DefaultTransportService and TransportServiceStub classes IMO

packages/device-management-kit/src/di.ts Outdated Show resolved Hide resolved
packages/device-management-kit/src/internal/transport/service/TransportService.ts Outdated Show resolved Hide resolved
packages/device-management-kit/src/internal/transport/service/TransportService.ts Outdated Show resolved Hide resolved
packages/device-management-kit/src/internal/transport/service/TransportService.stub.ts Outdated Show resolved Hide resolved
packages/transport/mockserver/src/MockserverTransport.ts Outdated Show resolved Hide resolved
packages/transport/web-hid/.prettierignore Outdated Show resolved Hide resolved
packages/transport/web-hid/src/api/transport/WebHidTransport.ts Outdated Show resolved Hide resolved
packages/transport/web-hid/src/api/transport/WebHidTransport.ts Outdated Show resolved Hide resolved
packages/device-management-kit/src/internal/transport/service/TransportService.ts Outdated Show resolved Hide resolved
packages/device-management-kit/src/internal/transport/service/TransportService.ts Outdated Show resolved Hide resolved
@valpinkman valpinkman force-pushed the feature/dsdk-558-extract-transport branch from 57d8b0d to cd277b5 Compare November 7, 2024 16:21
@valpinkman valpinkman force-pushed the feature/dsdk-558-extract-transport branch from cd277b5 to 55d2623 Compare November 7, 2024 16:47
@valpinkman valpinkman force-pushed the feature/dsdk-558-extract-transport branch from 55d2623 to fae2934 Compare November 8, 2024 10:16
@valpinkman valpinkman force-pushed the feature/dsdk-558-extract-transport branch from 79cce18 to fb76447 Compare November 8, 2024 11:18
@ofreyssinet-ledger
Copy link
Contributor

the PR title is wrong: it says transaction instead of transports

ofreyssinet-ledger
ofreyssinet-ledger approved these changes Nov 8, 2024
View reviewed changes
Copy link
Contributor

@ofreyssinet-ledger ofreyssinet-ledger left a comment

Choose a reason for hiding this comment

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

great work, thanks 👏

@ofreyssinet-ledger ofreyssinet-ledger changed the title ✨ (repo) [DSDK-558]: Extract transaction into their own packages ✨ (repo) [DSDK-558]: Extract transports into their own packages Nov 8, 2024
@valpinkman valpinkman force-pushed the feature/dsdk-558-extract-transport branch from fb76447 to 3491f55 Compare November 12, 2024 11:09
@valpinkman valpinkman force-pushed the feature/dsdk-558-extract-transport branch from 3491f55 to c21167e Compare November 12, 2024 14:00
@valpinkman valpinkman force-pushed the feature/dsdk-558-extract-transport branch from c21167e to 84e7879 Compare November 13, 2024 16:57
@valpinkman valpinkman force-pushed the feature/dsdk-558-extract-transport branch from 84e7879 to d1a0521 Compare November 14, 2024 17:17
@valpinkman valpinkman force-pushed the feature/dsdk-558-extract-transport branch from d1a0521 to 1d3a799 Compare November 14, 2024 17:30
@valpinkman valpinkman force-pushed the feature/dsdk-558-extract-transport branch from 1d3a799 to 0859764 Compare November 15, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdabbech-ledger jdabbech-ledger jdabbech-ledger approved these changes

@ofreyssinet-ledger ofreyssinet-ledger ofreyssinet-ledger approved these changes

Assignees

@valpinkman valpinkman

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@valpinkman @ofreyssinet-ledger @jdabbech-ledger