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.

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

Internalize QHeap dependency #3451

Merged
merged 9 commits into from
Jun 14, 2024
Merged

Internalize QHeap dependency #3451

merged 9 commits into from
Jun 14, 2024

Conversation

scorbajio
Copy link
Contributor

This pull request looks into the feasibility of replacing the qheap dependency with a local implementation. Also see #3450.

@scorbajio scorbajio added PR state: WIP dependencies Pull requests that update a dependency file package: client labels Jun 12, 2024
Copy link

codecov bot commented Jun 12, 2024

Codecov Report

Attention: Patch coverage is 74.66063% with 56 lines in your changes missing coverage. Please review.

Project coverage is 81.36%. Comparing base (d24ca11) to head (553e7da).
Report is 24 commits behind head on master.

Current head 553e7da differs from pull request most recent head 93cf10f

Please upload reports for the commit 93cf10f to get more accurate results.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
blockchain ?
client 84.31% <74.66%> (?)
common ?
statemanager ?
util ?
vm 63.04% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

@holgerd77
Copy link
Member

Hi there, thanks for picking this up! 👍 Just to clarify on how I used some terminology, so I used "internalize" to refer to "just copying over the code" and add the license (if the license permits, which - just confirmed for myself - is the case for the Apache license being used for the qheap package). As an example I referred to the crc.ts where we just did that, see also the comment header.

For qheap this is definitely also a viable option I guess, this is just a single file qheap.js which we can copy over, add the license, and also add some typing (judging from former experiences with other internalizations, this is amittedly the most tricky parts and this leaves some room for errors).

Yet another option might be to first look for a more modern but still simple, dependency-free and performant enough TypeScript alternative and then copy over the code rather from there. Not sure, if there is one this will likely be better, since - again - adding the typing is a bit hairy.

@holgerd77
Copy link
Member

👍

One thing I stumbled upon when coming back here along reviewing the cors internalization suggestion from @foufrix from here #3450 (comment) : we should not put the internalized code "somewhere" in the code but have a central and clearly marked place for this.

Can you please move over to a dedicated ext folder as we have done e.g. in devp2p here and place the qheap.ts file there together with an additional index.ts folder file? That has become somewhat our "standard" for these integrations. Thanks! 🙏

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

🎉

@holgerd77 holgerd77 merged commit 7debfaf into master Jun 14, 2024
33 of 34 checks passed
@holgerd77 holgerd77 deleted the qheap-experiment branch June 14, 2024 07:04
@holgerd77 holgerd77 changed the title Experiment with internalizing QHeap dependency Internalize QHeap dependency Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file package: client PR state: needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants