-
Notifications
You must be signed in to change notification settings - Fork 288
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
refactor(test-tooling): fix types of streams: use NodeJS.ReadableStream #3047
Merged
petermetz
merged 1 commit into
hyperledger-cacti:main
from
petermetz:refactor-test-tooling-nodejs-types-stream-usage
Mar 4, 2024
Merged
refactor(test-tooling): fix types of streams: use NodeJS.ReadableStream #3047
petermetz
merged 1 commit into
hyperledger-cacti:main
from
petermetz:refactor-test-tooling-nodejs-types-stream-usage
Mar 4, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
petermetz
requested review from
takeutak,
izuru0,
jagpreetsinghsasan,
VRamakrishna,
sandeepnRES and
outSH
as code owners
March 1, 2024 19:04
One comment: 🥵. In a more serious tone, great work @petermetz and @AndreAugusto11 |
jagpreetsinghsasan
approved these changes
Mar 4, 2024
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.
LGTM
petermetz
force-pushed
the
refactor-test-tooling-nodejs-types-stream-usage
branch
from
March 4, 2024 07:42
3f23bf1
to
8139132
Compare
outSH
requested changes
Mar 4, 2024
petermetz
force-pushed
the
refactor-test-tooling-nodejs-types-stream-usage
branch
from
March 4, 2024 17:01
8139132
to
edd28cc
Compare
1. The container management library that we use in the test infrastructure (called dockerode) is expecting streams that are defined in the global namespace of the `@types/node` library, e.g. the standard library of NodeJS itself. 2. Previously we were using the "streams" package to provide type information to the streams that we were passing around to dockerode and it was working fine, but after some changes that seem unrelated this has broken the compilation process. 3. The mentioned changes are not yet on the main branch, but we expect them to be there soon and so this change is laying the groundwork for that by pre-emptively fixing the broken build's root cause which is that the test-tooling package does not declare it's typings related dependencies correctly: It implicitly uses the NodeJS standard library's types but so far had not declared them on the package level. 4. This change is therefore to rectify the issue of the `@types/node` dependency missing from the test-tooling package and also the refactoring of some of the test ledger classes which were relying on the `streams` builtin package instead of correctly using the NodeJS.ReadableStream global. 5. Earlier the reasoning for this was that we try to avoid pulling in types from the global scope because we try to avoid any sort of dependency on the global scope in general. Once we have proof though that this is causing issues with the build, then we must give up the principle for practical reasons (and only in the minimum viable scope, e.g. this does not change the fact that everywhere else in the codebase we should still do our best to avoid using the global scoped classes, types, functions, etc..). Thank you to @AndreAugusto11 and @RafaelAPB for pointing out this issue through the pull request of his that is currently being worked on at the time of this writing: RafaelAPB#72 Related to but does not address hyperledger-cacti#2811 Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz
force-pushed
the
refactor-test-tooling-nodejs-types-stream-usage
branch
from
March 4, 2024 17:02
edd28cc
to
5b602f9
Compare
outSH
approved these changes
Mar 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(called dockerode) is expecting streams that are defined in the global
namespace of the
@types/node
library, e.g. the standard library of NodeJSitself.
to the streams that we were passing around to dockerode and it was working
fine, but after some changes that seem unrelated this has broken the
compilation process.
them to be there soon and so this change is laying the groundwork for that
by pre-emptively fixing the broken build's root cause which is that the
test-tooling package does not declare it's typings related dependencies
correctly: It implicitly uses the NodeJS standard library's types but
so far had not declared them on the package level.
@types/node
dependency missing from the test-tooling package and also the refactoring
of some of the test ledger classes which were relying on the
streams
builtin package instead of correctly using the NodeJS.ReadableStream global.
types from the global scope because we try to avoid any sort of dependency
on the global scope in general. Once we have proof though that this is
causing issues with the build, then we must give up the principle for
practical reasons (and only in the minimum viable scope, e.g. this does
not change the fact that everywhere else in the codebase we should still
do our best to avoid using the global scoped classes, types, functions,
etc..).
Thank you to @AndreAugusto11 and @RafaelAPB for pointing out this issue
through the pull request of his that is currently being worked on at the
time of this writing:
RafaelAPB#72
Related to but does not address #2811
Signed-off-by: Peter Somogyvari peter.somogyvari@accenture.com
Pull Request Requirements
upstream/main
branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-s
flag when usinggit commit
command. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.