Monorepo: Update lru-cache dependencies (ESM) / Switch Browser Test Provider #2804
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
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
holgerd77
changed the title
…er.spec.ts and eip4844.spec.ts for tx
… browser test provider, removed webdriverio
holgerd77
force-pushed
the
update-lru-and-other-deps
branch
from
c7a3e81 to
1e1bdf8
Compare
holgerd77
commented
Jun 21, 2023
| @@ -19,6 +19,7 @@ | |||
| "contributors": [ | |||
| "Alex Beregszaszi <alex@rtfs.hu>" | |||
| ], | |||
| "type": "commonjs", | |||
There was a problem hiding this comment.
I would want to keep this for now. It is generally not yet 100% clear what setting is appropriate here and on the VM browser tests work better when setting here to "commonjs"(lol).
Thenkei
reviewed
Jun 21, 2023
| // be some not so clean workaround. | ||
| // | ||
| // (note that @ts-ignore doesn't work since stripped on declaration (.d.ts) files) | ||
| protected _cache?: LRUCache<string, any> |
There was a problem hiding this comment.
Suggested change
| protected _cache?: LRUCache<string, any> | |
| protected _cache?: LRUCache<string, Uint8Array> |
Is this enough in your case?
There was a problem hiding this comment.
No, we need undefined as a potential value within the current code setup, this is not allowed anymore by the lru-cache library, therefore the any typing as some kind of hack here.
acolytec3
previously approved these changes
Jun 21, 2023
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.
This PR updates different require-using dependencies, in particular the lru-cache dependency.
I've now "solved" the undefined/null-now-forbidden question for lru-cache by just using
@ts-ignore, see also comment on trie library for some reasing, also opened this issue isaacs/node-lru-cache#308 on thelru-cachelibrary, since - while also having some intuition why restricting on undefined and null makes sense - I also feel that e.g. for our use case this is still the cleanest way to do things.Update:
@ts-ignoredoesn't work since it is stripped in the declaration files in thedistfolder. I am now usinganyfor this. Still feel this is the better solution than using an"UNDEFINED"string or whatever one could come up with.