-
Notifications
You must be signed in to change notification settings - Fork 25
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
[Chore][DEVX-476] Improve README file #841
base: master
Are you sure you want to change the base?
Conversation
- improve readme file
🦋 Changeset detectedLatest commit: ed01a97 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #841 +/- ##
=======================================
Coverage 93.07% 93.07%
=======================================
Files 25 25
Lines 289 289
Branches 14 14
=======================================
Hits 269 269
Misses 20 20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
- improve readme file
- add patch release changeset
@@ -150,11 +154,13 @@ const client = new ClientBuilder() | |||
.build() | |||
``` | |||
|
|||
> [!WARNING] | |||
> Do not add the built-in middlewares using `withMiddleware` method. Adding by this method does not respect the ordering of the middlewares and could lead to unexpected behavior. | |||
<!-- > [!WARNING] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not true anymore? I still see the ordering is taken into account in this method:
const middlewares = this.middlewares.slice() |
Will it work now if I add middlewares in any order? Or is this ordering somewhere written?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I mentioned it that when using only the withMiddleware()
method the order matters, hence the need to use only the withMiddleware()
method to add all middleware to the call chain.
If you see in the example, all the middleware were added using the withMiddleware()
method whilst the second example shows what would happen if this middleware methods are mixed.
Maybe I need to make these points clearer and elaborate more on this concept. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
withMiddleware
method can be used to add middleware functions (both built-in and custom middleware) in an ordered fashion.
From this sentence I understand that withMiddleware
adds the middlewares in some order, but it is not clear what is the order or who creates this order.
I think the main point here is that the middlewares would not work correctly if added using withMiddleware()
in random order. There is no section about how the built-in middlewares should be ordered to work correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I can see what you mean now. I will create a task to change this behaviour in the SDK. We have to find a way to allow the customers to add custom middleware at any point in the execution chain. We always had this feature pre SDK v2.
Summary
Improve SDK
README.md
file, remove and/or update outdated code and documentationsCompleted Tasks
Jira Ticket
DEVX-476