chore: use primordials #216
Open
+376
−197
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
|
CC @isaacs |
MoLow
commented
Jul 4, 2023
isaacs
reviewed
Jul 5, 2023
src/primordials.ts
Outdated
| @@ -0,0 +1,4 @@ | |||
| export const primordials: any = // TODO export as typeof import('node-primordials'), types are wrong | |||
| // @ts-expect-error | |||
| typeof primordials !== 'undefined' ? primordials /* c8 ignore next */ : | |||
There was a problem hiding this comment.
I don't think c8 ignore next is a thing, is that new? I thought it only supported /* c8 ignore start */ and /* c8 ignore stop */
There was a problem hiding this comment.
it is. you can also specify the amount of lines. https://github.com/bcoe/c8/blob/main/README.md#ignoring-uncovered-lines-functions-and-blocks
There was a problem hiding this comment.
Oh, that's somewhat new-ish, I think. (Like, added in the last 5 years or so probably lol.) Nice!
|
Test failures are spurious, should be fixed by rebasing onto main to update tap. |
MoLow
force-pushed
the
start-primodials
branch
3 times, most recently
from
184993c to
aba228e
Compare
|
@isaacs PTAL |
isaacs
reviewed
Jul 11, 2023
isaacs
reviewed
Jul 11, 2023
| '$', | ||
| '\\', | ||
| '!', | ||
| ]) |
There was a problem hiding this comment.
Does SafeSet only take arrays, not arbitrary iterables?
There was a problem hiding this comment.
see the implementation here https://github.com/isaacs/node-primordials/pull/9/files# I guess it would work with any iterable, but didn't try
|
Looks mostly fine. I'd like to get the PRs on node-primordials landed before landing this, so that it can be a simple dep update. Then the real question will be what the perf impact is, if any. If it's awful, we'll have to figure out some way to either mitigate it, or have the primordials version live side by side somehow with the normal code. |
|
ping @isaacs can you take a look? |
|
@isaacs as we discussed - I'v ran benchmarks on the glob repo: isaacs/node-glob@main...MoLow:node-glob:primordials |
Yoggie9477
approved these changes
Sep 28, 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.
this is just a POC to start the process of node exposing
fs.glob.if this PR will land, I will complete the process on all the rest of the code.
also, it seems like
node-primordialsneeds some type fixes / additionsdepends on isaacs/node-primordials#8 and isaacs/node-primordials#9