Skip to content

Conversation

rluvaton
Copy link
Member

@rluvaton rluvaton commented Jun 29, 2023

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jun 29, 2023
@rluvaton rluvaton changed the title fs: add async glob fs: add async glob and make it (async) iteratable Jun 29, 2023
}
let val;
try {
val = await readdir(path, { __proto__: null, withFileTypes: true });
Copy link
Member

Choose a reason for hiding this comment

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

use opendir instead of readdir

Copy link
Member Author

Choose a reason for hiding this comment

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

and yield the results or just opendir? because yielding the result will cause incomplete cache

Copy link
Member

Choose a reason for hiding this comment

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

then it probably makes sense to use readdir for the first iteration

Copy link
Member Author

@rluvaton rluvaton Jun 29, 2023

Choose a reason for hiding this comment

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

how will it solve the issue? on further iterations we should get from the cache and not use opendir anyway...

Copy link
Member

Choose a reason for hiding this comment

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

I meant first iteration of the feature

Comment on lines 465 to 468
children = await this.#cache.readdir(fullpath);
}

for (let i = 0; i < children.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
children = await this.#cache.readdir(fullpath);
}
for (let i = 0; i < children.length; i++) {
children = this.#cache.opendir(fullpath);
}
for await (let i = 0; i < children.length; i++) {

changing to opendir will require this

Copy link
Member Author

Choose a reason for hiding this comment

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

what about the cache?

Copy link
Member Author

Choose a reason for hiding this comment

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

doing that we will have incomplete cache

for (const index of pattern.indexes) {
// For each child, check potential patterns
if (this.#cache.seen(entryPath, pattern, index) || this.#cache.seen(entryPath, pattern, index + 1)) {
// TODO - fix this not finish the addSubPatterns
Copy link
Member Author

Choose a reason for hiding this comment

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

@MoLow any JS trick to finish the called generator?

Copy link
Member

Choose a reason for hiding this comment

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

test the return value of addFromChildren

for (const entry of children) {
	const result = this.addFromChildren(entry, path, fullpath, pattern, last, isLast);
	if (result === null) {
		return;
	}
	yield result;
}

Copy link
Member Author

@rluvaton rluvaton Jun 29, 2023

Choose a reason for hiding this comment

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

addFromChildren is a generator by itself so I need to get the value from it so I thought if you have another way

this.#cache.addToStatCache(join(fullpath, entry.name), entry);
const children = await this.#cache.readdir(fullpath);

for (const entry of children) {
Copy link
Member

Choose a reason for hiding this comment

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

dont use for of, since the prototype can be tampered

Copy link
Member Author

Choose a reason for hiding this comment

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

can I use for of for going over generator or do I need to handle all the state by myself?

@rluvaton rluvaton requested a review from MoLow June 29, 2023 11:30
@rluvaton rluvaton closed this Jun 25, 2024
@rluvaton rluvaton deleted the add-async-glob branch June 25, 2024 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants